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

Upgrade Rubocop/Erblint and fix cop violations #1803

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

murny
Copy link
Contributor

@murny murny commented Aug 6, 2020

Replaces #1749

Upgrade rubocop and erblint gems to latest versions

Fix code to make new cops happy, mainly Style/ArrayCoercion and Style/StringConcatenation

@@ -93,6 +93,12 @@ Rails:
Rails/FindEach:
Enabled: false

Rails/SkipsModelValidations:
Exclude:
- app/controllers/aip/v1/collections_controller.rb
Copy link
Contributor Author

@murny murny Aug 6, 2020

Choose a reason for hiding this comment

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

This cop wants us to avoid using .insert because it skips validations. But it's a false positive as we using .insert from our rdf graph object instead of ActiveRecord.

app/controllers/aip/v1/collections_controller.rb:24:31: C: Rails/SkipsModelValidations: 
Avoid using insert because it skips validations. 
(https://guides.rubyonrails.org/active_record_validations.html#skipping-validations)

      rdf_graph_creator.graph.insert(RDF::Statement(statement_definition))
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So just disable this cop from these files

@@ -7,7 +7,7 @@ def display
community_id, collection_id = @value.split('/')
community_title = Community.find(community_id).title
collection_title = if collection_id
'/' + Collection.find(collection_id).title
"/#{Collection.find(collection_id).title}"
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 prefers string interpolation over string concatenation. Updates all our violations of this.

app/decorators/facets/member_of_paths.rb:10:26: C: 
Style/StringConcatenation: Prefer string interpolation to string concatenation. 
(https://rubystyle.guide#string-interpolation)
                         ‘/’ + Collection.find(collection_id).title
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@@ -180,7 +180,7 @@ def indexed_model_name(name)
# the basic DSL for declaring Solr indexes who will take their contents from attributes
# declared on the objects we will export
def index(attr, role:, type: :string)
role = [role] unless role.is_a? Array
role = Array(role)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

app/models/exporters/solr/base_exporter.rb:183:7: C: 
Style/ArrayCoercion: Use Array(role) instead of explicit Array check. 
(https://rubystyle.guide#array-coercion)
      role = [role] unless role.is_a? Array
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Interesting new cop. Essentially it argues you should Use Array() instead of explicit Array check or [*var], when dealing with a variable you want to treat as an Array, but you’re not certain it’s an array.

So switched any violations over to new syntax

Copy link
Contributor

Choose a reason for hiding this comment

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

Going to suggest that we disable this cop/revert these changes (I vaguely think there might have been an older cop we disabled for the same reason?). Array() is kind of a subtle foot-gun, and I don't think Batsov understands that...

The difference is this:

irb(main):001:0> a = {:a => 1, :b => 2}
=> {:a=>1, :b=>2}
irb(main):002:0> array = [a] unless a.is_a? Array
=> [{:a=>1, :b=>2}]
irb(main):003:0> different_array = Array(a)
=> [[:a, 1], [:b, 2]]

Instead of putting a hash in an array, it blows it up into keywise paired subarrays, which generally isn't what people expect, making it not semantically equivalent to the [x] unless x.is_a? Array pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. We can play it safe here and revert these 👍

@@ -180,7 +180,7 @@ def indexed_model_name(name)
# the basic DSL for declaring Solr indexes who will take their contents from attributes
# declared on the objects we will export
def index(attr, role:, type: :string)
role = [role] unless role.is_a? Array
role = Array(role)
Copy link
Contributor

Choose a reason for hiding this comment

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

Going to suggest that we disable this cop/revert these changes (I vaguely think there might have been an older cop we disabled for the same reason?). Array() is kind of a subtle foot-gun, and I don't think Batsov understands that...

The difference is this:

irb(main):001:0> a = {:a => 1, :b => 2}
=> {:a=>1, :b=>2}
irb(main):002:0> array = [a] unless a.is_a? Array
=> [{:a=>1, :b=>2}]
irb(main):003:0> different_array = Array(a)
=> [[:a, 1], [:b, 2]]

Instead of putting a hash in an array, it blows it up into keywise paired subarrays, which generally isn't what people expect, making it not semantically equivalent to the [x] unless x.is_a? Array pattern.

@murny murny force-pushed the rubocop-upgrade branch from 4d135c2 to 62db4c4 Compare August 9, 2020 22:55
Copy link
Member

@pgwillia pgwillia left a comment

Choose a reason for hiding this comment

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

@murny murny merged commit d72c830 into integration_postmigration Aug 11, 2020
@murny murny deleted the rubocop-upgrade branch August 11, 2020 05:27
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.

3 participants