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

Move gems to Gemfile and improve rubocop config #6

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

mwolman
Copy link
Collaborator

@mwolman mwolman commented Sep 26, 2023

In this PR, I'm implementing the following improvements:

  • Improve the .rubocop.yml file and fix multiple issues
    • I also applied a refactoring to the Outboxable class and spec in order to fix some issues
  • Added RuboCop to our CI checks
  • Upgrade minor and patch versions of some gems
  • Move some dependencies from the gemspec to the Gemfile to follow good practices:

Screenshot 2023-09-26 at 15 15 58

Copy link
Collaborator

@guillermoap guillermoap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the refactoring and best practices introduced in this PR, left some suggestions. Let me know what you think

lib/active_outbox/outboxable.rb Outdated Show resolved Hide resolved
lib/active_outbox/outboxable.rb Outdated Show resolved Hide resolved
lib/active_outbox/outboxable.rb Show resolved Hide resolved
lib/active_outbox/outboxable.rb Outdated Show resolved Hide resolved
lib/active_outbox/outboxable.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,269 @@
# frozen_string_literal: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this change be a renaming git change? this way we are losing the history of the file. Try using git mv spec/lib/outboxable_spec.rb spec/lib/active_outbox/outboxable_spec.rb

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried what you mentioned, but it didn't work 😕 it has too many changes, so it recognizes it as a new file

Copy link
Contributor

@santiagodiaz santiagodiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@mwolman mwolman force-pushed the move-gems-to-gemfile branch 2 times, most recently from f3d1a6f to d38609c Compare October 3, 2023 19:13
Copy link
Collaborator

@guillermoap guillermoap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are looking great, once we fix the git mv issue with the spec file I think this is good to go 🚀

@mwolman mwolman merged commit 61fe6b7 into main Oct 4, 2023
4 of 5 checks passed
@mwolman mwolman deleted the move-gems-to-gemfile branch October 4, 2023 17:37
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

3 participants