-
Notifications
You must be signed in to change notification settings - Fork 5
153 add rubocop for linting uniformity #156
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
base: main
Are you sure you want to change the base?
Conversation
5ddfe21
to
eea8e55
Compare
Lint/UnderscorePrefixedVariableName: | ||
Exclude: | ||
- 'lib/decanter/core.rb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not want to mess with functionality in this PR, so I opted to exclude certain rubocop configs on core.rb
that would require more invasive changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the general premise of not doing those refactorings within this PR, but I might prefer to actually address the Lint/UnderscorePrefixedVariableName
errors within this PR, since it just involves renaming variables without changing any logic, rather than disabling the Rubocop rule. I'm good with handling the other violations separately in this new issue.
spec.add_development_dependency 'rake', '~> 12.0' | ||
spec.add_development_dependency 'rspec-rails', '~> 3.9' | ||
spec.add_development_dependency 'simplecov', '~> 0.15.1' | ||
spec.metadata['rubygems_mfa_required'] = 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoledow not sure if MFA is already required for publishing this gem but rubocop is requiring it. Seems like a good idea.
22bdbe2
to
5661f3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces RuboCop-based linting to enforce code uniformity and applies auto-corrections across the codebase. Key changes include adding RuboCop as a dependency with an accompanying configuration, replacing string concat calls with interpolation, and refining parser error messaging and behavior.
Reviewed Changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
lib/decanter/parser/datetime_parser.rb | Simplified nil/empty string check |
lib/decanter/parser/date_parser.rb | Simplified nil/empty string check |
lib/decanter/parser/core.rb | Updated allowed class checking with safe navigation |
lib/decanter/parser/compose_parser.rb | Adjusted error message formatting |
lib/decanter/parser/boolean_parser.rb | Refined BooleanParser logic with a standardized true-values array |
lib/decanter/parser/array_parser.rb | Standardized error message formatting |
lib/decanter/parser.rb | Updated error handling and messaging |
lib/decanter/extensions.rb | Simplified save call in update methods |
lib/decanter/core.rb | Reformatted key-handling and error message formatting |
lib/decanter/configuration.rb | Minor formatting changes |
lib/decanter/collection_detection.rb | Adjusted collection option validation |
lib/decanter/base.rb | Added frozen string literal and minor dependency updates |
lib/decanter.rb | Updated decanter lookup and error handling |
decanter.gemspec | Adjusted file inclusion and dependency order |
Rakefile | Added tasks for RSpec and RuboCop |
Gemfile | Updated dependency group with RuboCop and testing tools |
.rubocop.yml | New RuboCop configuration enforcing linting standards |
.github/workflows/versionci.yml | Updated CI workflow to run RuboCop and RSpec tests |
Comments suppressed due to low confidence (1)
lib/decanter/core.rb:54
- Using an unnamed splat parameter in the 'ignore' method could lead to unexpected behavior when pushing arguments. Consider naming the parameter (e.g., *args) and updating its usage accordingly to ensure proper handling of the input.
def ignore(*)
Lint/UnderscorePrefixedVariableName: | ||
Exclude: | ||
- 'lib/decanter/core.rb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the general premise of not doing those refactorings within this PR, but I might prefer to actually address the Lint/UnderscorePrefixedVariableName
errors within this PR, since it just involves renaming variables without changing any logic, rather than disabling the Rubocop rule. I'm good with handling the other violations separately in this new issue.
Metrics/AbcSize: | ||
Max: 20 | ||
Exclude: | ||
- 'lib/decanter/core.rb' | ||
|
||
Metrics/BlockLength: | ||
Exclude: | ||
- 'spec/**/*_spec.rb' | ||
- 'decanter.gemspec' | ||
|
||
Metrics/CyclomaticComplexity: | ||
Exclude: | ||
- 'lib/decanter/core.rb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to disable the specific methods/blocks that are violating the rules. Then, if that code is modified before #158 is resolved, any new code will be subject to the linting rules.
"#{klass_or_sym.name}Decanter" | ||
when Symbol | ||
klass_or_sym.to_s.singularize.camelize | ||
"#{klass_or_sym.to_s.singularize.camelize}Decanter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting - why did you have to add "Decanter" to the end of this string?
parser do |val, _options| | ||
next if val.nil? || val == '' | ||
|
||
true_values = [1, '1', true, 'true', 'TRUE', 'True'] | ||
true_values.include?(val) || val.to_s.downcase == 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking but there might be some additional refactoring opportunities in this block. If we downcase the val (if it's a string), we don't have to check both 'TRUE'
and 'true'
.
Items Addressed
Add RuboCop for Linting Uniformity
Changes
'rubocop', '~> 1.59'
to development and test dependenciesbundle exec rubocop -A
(autocorrect on all files)5.1.0
rubocop