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

[DEVX-4047] Bump rubocop to latest and fix offenses #118

Merged
merged 1 commit into from
May 13, 2024
Merged

Conversation

lstrzebinczyk
Copy link
Contributor

Updated rubocop, rubocop-rspec and rubocop-rails to the latest version, removed all of the rule overwriting and fixed the violations. Disabled 3 rules, which proved to be time consuming to fix.

Review

  • All tests are passing.
  • Get approval.

Pre-merge checklist

  • The PR relates to a single subject with a clear title and description in grammatically correct, complete sentences.
  • Verify that feature branch is up-to-date with master (if not - rebase it).
  • Double check the quality of commit messages.
  • Squash related commits together.

@lstrzebinczyk lstrzebinczyk self-assigned this May 9, 2024
@lstrzebinczyk lstrzebinczyk requested a review from a team as a code owner May 9, 2024 16:04
@lstrzebinczyk
Copy link
Contributor Author

@toptal-anvil ping reviewers

@lstrzebinczyk lstrzebinczyk force-pushed the devx-4047 branch 2 times, most recently from 65cfb00 to 042298f Compare May 10, 2024 08:36
@kml kml requested a review from a team May 10, 2024 08:38
subject.email = login
subject.save!
user.email = login
user.save!
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop asked me to, and even though it incorrectly applied it's rule, I feel like this is a good move.

@@ -11,7 +9,7 @@ def self.defaults!
path = CONFIG_DEFAULT.to_s
hash = ConfigLoader.load_file(path)
config = Config.new(hash, path).tap(&:make_excludes_absolute)
puts "configuration from #{path}" if ConfigLoader.debug?
Rails.logger.debug { "configuration from #{path}" } if ConfigLoader.debug?
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about using global logger?

Suggested change
Rails.logger.debug { "configuration from #{path}" } if ConfigLoader.debug?
Granite::Form.config.logger.debug { "configuration from #{path}" } if ConfigLoader.debug?

I do not see much usage of loggers in the repository but anyway it would be good to keep it generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an automatic change by rubocop. We can discuss further changes as separate tickets @mbie

@lstrzebinczyk lstrzebinczyk merged commit 5ad5f54 into master May 13, 2024
5 checks passed
@lstrzebinczyk lstrzebinczyk deleted the devx-4047 branch May 13, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants