Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement full PR review #11

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

aneshodza
Copy link
Contributor

@aneshodza aneshodza commented Jun 20, 2023

Making a todo list so i dont lose oversight:

  • Newlines around headers in readme
  • Remove , nil from ENV.fetch
  • Move CLIENT into init.rb
  • Extract verify_signature into before_action with guard clause
  • Use strong params in webhooks controller
  • This
  • Use Client.compare instead of .commits
  • Delete WebhooksHelper
  • Extract state in pull_request.rb into a method
  • Extract params[:pull_request] in pull_request.rb
  • Change multi line string to partial in render
  • get_ and set_ in hook_listener
  • Use Rails.root.join instead of File.expand_path("#{File.dirname(__FILE__)}
  • This
  • Use i18n everywhere
  • Change time zone to local
  • Add an installation guide to the readme
  • Extend ENV instead of overwriting

@aneshodza aneshodza self-assigned this Jun 20, 2023
@aneshodza
Copy link
Contributor Author

Not sure if strong params make sense, as we have the signatures

@aneshodza
Copy link
Contributor Author

@sihu FYI, this PR is still open. Might make sense to let someone else take this over

@aneshodza aneshodza removed their assignment Aug 23, 2023
@sihu sihu self-assigned this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants