Rubocop linting and performance violations #948
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
solebared
commented
Apr 21, 2021
| @@ -52,9 +52,9 @@ def contribution_blueprint_options | |||
| end | |||
|
|
|||
| def filter_params | |||
| return {} unless allowed_params&.to_h.any? | |||
| return {} unless allowed_params.to_h.any? | |||
There was a problem hiding this comment.
I don't think allowed_params can be nil?
There was a problem hiding this comment.
Right. It can be empty/blank or it can raise an error, but it can't be nil. This may have been left over from a previous iteration where that wasn't the case
Comment on lines
-23
to
-33
| def given_inputs | ||
| # FIXME: ActiveInteraction::Base#given? seems to have a bug recognizing multipart date params | ||
| raw_inputs = @_interaction_inputs | ||
| inputs.select do |key| | ||
| given?(key) || ( | ||
| raw_inputs.key?(:"#{key}(1i)") && | ||
| raw_inputs.key?(:"#{key}(2i)") && | ||
| raw_inputs.key?(:"#{key}(3i)") | ||
| ) | ||
| end | ||
| end |
There was a problem hiding this comment.
Moved to the end of the file.
| @@ -543,56 +543,56 @@ | |||
| # end | |||
|
|
|||
| # rubocop:enable Layout/CommentIndentation | |||
| end | |||
There was a problem hiding this comment.
Everything below here in this file is just whitespace change; moved the class definitions below out of the block above.
| @@ -1,7 +1,7 @@ | |||
| class AddAutoGeneratedToCommunicationLog < ActiveRecord::Migration[6.0] | |||
| def up | |||
| add_column :communication_logs, :auto_generated, :boolean | |||
| change_column_default :communication_logs, :auto_generated, :false | |||
| change_column_default :communication_logs, :auto_generated, false | |||
There was a problem hiding this comment.
Fixing but :false seems to set the default correctly.
| @@ -7,13 +7,14 @@ | |||
| importer = Importers::SubmissionResponseImporter.new(User.first, csv) | |||
| importer.import(path) | |||
|
|
|||
| # TODO process offers and old_offers as well | |||
There was a problem hiding this comment.
I've tried to interpret the meaning of the pre-existing comment here.
24 tasks
solebared
force-pushed
the
rubocop/semantic-blocks
branch
from
77961b8
to
eef7c99
Compare
solebared
force-pushed
the
rubocop/semantic-blocks
branch
from
eef7c99
to
f422ae3
Compare
solebared
force-pushed
the
rubocop/semantic-blocks
branch
from
f422ae3
to
be847ac
Compare
Some instances require more analysis or knowledge so have left todos instead of fixing. https://docs.rubocop.org/rubocop/cops_lint.html#lintuselessassignment
Some of these instances didn't appear to need safe navigation. Others were smells, IMO, so left as todos. https://docs.rubocop.org/rubocop/cops_lint.html#lintsafenavigationchain
Fixing violations of the following: * Lint/BooleanSymbol * Lint/Debugger * Lint/DuplicateMethods * Lint/IneffectiveAccessModifier * Lint/RedundantStringCoercion
|
Reviewed in person with @svileshina . |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Building on #935, fixes violations of
Lint/*andPerformance/*rules.What
See individual commits for the list of rule violations addressed. Diffs probably also make more sense one commit at a time.
In many instances, i left
rubocop:todos instead of fixing the violation (this is likerubocop:disablebut reveals that we really want to fix the violation instead of disable it).See inline PR comments for specific questions.
Testing
Relying on our existing suite to catch any regressions.
Next Steps
?
Outstanding Questions, Concerns and Other Notes
?
Pre-Merge Checklist