Introduce new cops from newer rubocop #39527

fatkodima · 2020-06-03T16:51:28Z

No description provided.

rails-bot · 2020-09-01T17:10:48Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

KapilSachdev · 2020-09-05T12:46:31Z

@fatkodima Do you think we can bump to 0.90 in this PR ?

fatkodima · 2020-09-05T12:50:53Z

There were a lot of new cops introduced between 0.85 and 0.90. So, probably, it is more tractable to rebase and merge this and then do 0.85 -> 0.90 separately?

KapilSachdev · 2020-09-05T14:19:47Z

Sounds good 👍.
I was thinking if version bump can be reviewed once rather twice.
Two PRs are always good and easier to review but in case we bump in this one, maybe there are reasonble amount of changes introduced that can be reviewed in one go.

fatkodima · 2020-09-05T22:26:49Z

Made an additional commit with updating 0.85 => 0.90.

jonathanhefner

Thank you for working on this @fatkodima! 🙌

actionpack/test/controller/parameters/accessors_test.rb

actionpack/test/controller/parameters/parameters_permit_test.rb

actionpack/test/dispatch/header_test.rb

jonathanhefner · 2020-09-06T14:11:08Z

actiontext/lib/action_text/content.rb

-        fragment = ActionText::AttachmentGallery.fragment_by_canonicalizing_attachment_galleries(fragment)
-        fragment


I'm not sure, but this might be an intentional stylistic choice.

activesupport/lib/active_support/core_ext/digest/uuid.rb

jonathanhefner · 2020-09-06T16:18:22Z

activesupport/test/hash_with_indifferent_access_test.rb

@@ -527,7 +527,7 @@ def test_indifferent_hash_with_array_of_hashes
    hash = { "urls" => { "url" => [ { "address" => "1" }, { "address" => "2" } ] } }.with_indifferent_access
    assert_equal "1", hash[:urls][:url].first[:address]

-    hash = hash.to_hash
+    hash = hash.to_hash # rubocop:disable Style/RedundantSelfAssignment


Seems like this is a false positive?

Yeah, this is a false positive.
I will fix this and most other mentioned false positives in upstream rubocop. Wdyt about leaving this as inline for now?

My concern is that any new code similar to this would also have to add the pragma to keep Rubocop happy. And we don't know exactly when Rubocop will be fixed, released, and updated in Rails, so my opinion is that it's best to remove it until Rubocop is fixed.

.rubocop.yml

jonathanhefner · 2020-09-06T16:46:20Z

.rubocop.yml

+Lint/DeprecatedOpenSSLConstant:
+  Enabled: true


This is very useful! 💪 I still hold hope for ruby/openssl#377. 🤞

I do prefer to disscuss about it in #39410, #39504, #39540.

jonathanhefner · 2020-09-06T16:51:47Z

.rubocop.yml

+Style/RedundantRegexpEscape:
+  Enabled: true


I love this rule! 😍

composerinteralia · 2020-09-06T17:59:17Z

It looks like we bumped the version of Rubocop in #40185

fatkodima · 2020-09-06T18:16:18Z

Ah, I see.
What is the point of just bumping rubocop without actually any changes to the code?

jonathanhefner · 2020-09-06T18:22:11Z

I think it was to help the Debian maintainers. At least, that is my impression. Regardless, I think this PR worth pursuing. 👍

fatkodima · 2020-09-06T18:40:02Z

@jonathanhefner thanks for review! Sorry for the noise, I'm for sure should have more carefully reviewed my autocorrected changes, esp. those testing fetch/dig 😄

Updated with most of your suggestions.

kamipo · 2020-09-06T20:59:56Z

Can you explain more about how much it's worth it to add some new style cops?

Note that we don't follow The Ruby Style Guide.

fatkodima · 2020-09-06T21:10:22Z

Can you explain more about how much it's worth it to add some new style cops?

Is this rhetorical question?
(Likely) better, simpler, faster, safer, consistent, less buggy, etc code.

kamipo · 2020-09-06T21:32:53Z

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

          def extract_foreign_key_action(specifier)
-            case specifier
-            when "c"; :cascade


It looks like a cosmetic change.

kamipo · 2020-09-06T21:37:34Z

activestorage/app/models/active_storage/blob.rb

+    def create_and_upload!(io:, filename:, key: nil, content_type: nil, metadata: nil, service_name: nil, identify: true, record: nil)
      create_after_unfurling!(key: key, io: io, filename: filename, content_type: content_type, metadata: metadata, service_name: service_name, identify: identify).tap do |blob|


It was originally intended to arrange the order of the keys, now the intent is broken by rubocop autocorrect.

jonathanhefner

Round 2! 👀

jonathanhefner · 2020-09-06T20:41:29Z

activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb

+        FOREIGN_KEY_ACTIONS = {
+          "CASCADE" => :cascade,
+          "SET NULL" => :nullify,
+          "RESTRICT" => :restrict
+        }
+
        def extract_foreign_key_action(specifier)
-          case specifier
-          when "CASCADE"; :cascade
-          when "SET NULL"; :nullify
-          when "RESTRICT"; :restrict
-          end
+          FOREIGN_KEY_ACTIONS[specifier]


I'm still not sure that this change makes the code more readable. It is more clear than an inline Hash, but it breaks up the locality without an obvious reason why, since FOREIGN_KEY_ACTIONS isn't used elsewhere.

But that's just my opinion. Other people may disagree. 😄

jonathanhefner · 2020-09-06T21:10:04Z

activesupport/test/hash_with_indifferent_access_test.rb

@@ -527,7 +527,7 @@ def test_indifferent_hash_with_array_of_hashes
    hash = { "urls" => { "url" => [ { "address" => "1" }, { "address" => "2" } ] } }.with_indifferent_access
    assert_equal "1", hash[:urls][:url].first[:address]

-    hash = hash.to_hash
+    hash = hash.to_hash # rubocop:disable Style/RedundantSelfAssignment


My concern is that any new code similar to this would also have to add the pragma to keep Rubocop happy. And we don't know exactly when Rubocop will be fixed, released, and updated in Rails, so my opinion is that it's best to remove it until Rubocop is fixed.

jonathanhefner · 2020-09-06T21:24:14Z

actionpack/test/controller/parameters/parameters_permit_test.rb

-    values += [STDOUT, StringIO.new, ActionDispatch::Http::UploadedFile.new(tempfile: __FILE__),
+    values += [$stdout, StringIO.new, ActionDispatch::Http::UploadedFile.new(tempfile: __FILE__),


I'm not sure about this one. The intent was actually STDOUT, but I don't know if it makes a difference.

jonathanhefner · 2020-09-06T21:27:26Z

activesupport/lib/active_support/testing/stream.rb

-          silence_stream(STDOUT) do
-            silence_stream(STDERR) do
+          silence_stream($stdout) do
+            silence_stream($stderr) do


These should not be changed.

kamipo · 2020-09-06T21:42:18Z

activerecord/lib/active_record/persistence.rb

-        if id.is_a?(Array)
+        case id
+        when Array


It looks like a cosmetic change.
That is the reason that originaly if id.is_a?(Array) was used is elsif id == :all didn't exist before.

kamipo · 2020-09-06T21:47:54Z

actioncable/lib/action_cable/server/configuration.rb

@@ -24,7 +24,7 @@ def initialize
      # If the adapter cannot be found, this will default to the Redis adapter.
      # Also makes sure proper dependencies are required.
      def pubsub_adapter
-        adapter = (cable.fetch("adapter") { "redis" })
+        adapter = (cable.fetch("adapter", "redis"))


I don't want to enable such like unsafe autocorrect.
Whether a block is redundant depends on whether the receiver is a hash.

jonathanhefner · 2020-09-06T21:53:34Z

@kamipo I agree we should be judicious about which Style rules we adopt. There are some rules here which might be better to omit (e.g. Style/HashLikeCase), but overall I think the changes are positive.

Perhaps we should only keep Style rules that have an immediate positive effect on the current code base (e.g. Style/RedundantRegexpEscape).

kamipo · 2020-09-06T22:14:41Z

Can you explain more about how much it's worth it to add some new style cops?

Is this rhetorical question?
(Likely) better, simpler, faster, safer, consistent, less buggy, etc code.

As a most active contributor in https://github.com/rails/rails/blob/master/.rubocop.yml,
I basically explain why the cop should be enabled, what is worth (mostly a reduction in review costs).

#36472
4353ada
6b8daad
7ebfb31
#34904
6d95901
#34764
f907b41
b2c1e29

Since some your changes have little explanation, I feel it costs a lot to review it than more explained changes.

I hope you can explain more in the future.

fatkodima · 2020-09-07T07:26:36Z

Agreed, it is better to introduce some of them one by one, with better and more detailed explanations. If someone interested in this, please do.

KapilSachdev · 2020-09-07T10:11:14Z

@fatkodima you have already invested much time in this one, so i think it would be better if you update this PR and remove "controversial" cops as of now.
But if you still don't want to, I can start adding cop in that case.

fatkodima · 2020-09-07T10:15:07Z

But if you still don't want to, I can start adding cop in that case.

Please, do. You can take ownership of this pr without any doubts.

fatkodima requested a review from rafaelfranca as a code owner June 3, 2020 16:51

rails-bot bot added actionmailbox actionpack actionview activejob activerecord activesupport labels Jun 3, 2020

jonathanhefner mentioned this pull request Jun 30, 2020

WIP: ENV access clean ups #39765

Closed

richardkmichael mentioned this pull request Jun 30, 2020

Style/RedundantFetchBlock causes undefined method error. rubocop/rubocop#8227

Closed

rails-bot bot added the stale label Sep 1, 2020

jonathanhefner removed the stale label Sep 1, 2020

fatkodima force-pushed the update-rubocop branch 2 times, most recently from b31a84d to c7d1d5b Compare September 5, 2020 19:14

rails-bot bot added actioncable actiontext labels Sep 5, 2020

fatkodima force-pushed the update-rubocop branch 2 times, most recently from a58d6c2 to 6186a9c Compare September 5, 2020 22:13

jonathanhefner reviewed Sep 6, 2020

View reviewed changes

Introduce new cops from newer rubocop

00f944e

fatkodima force-pushed the update-rubocop branch from 6186a9c to 00f944e Compare September 6, 2020 18:33

fatkodima changed the title ~~Update rubocop to 0.85~~ Introduce new cops from newer rubocop Sep 6, 2020

kamipo reviewed Sep 6, 2020

View reviewed changes

jonathanhefner reviewed Sep 6, 2020

View reviewed changes

kamipo reviewed Sep 6, 2020

View reviewed changes

fatkodima closed this Sep 7, 2020

kamipo mentioned this pull request Sep 8, 2020

feat(rubocop): Add new cops #40193

Closed

		fragment = ActionText::AttachmentGallery.fragment_by_canonicalizing_attachment_galleries(fragment)
		fragment

		def create_and_upload!(io:, filename:, key: nil, content_type: nil, metadata: nil, service_name: nil, identify: true, record: nil)
		create_after_unfurling!(key: key, io: io, filename: filename, content_type: content_type, metadata: metadata, service_name: service_name, identify: identify).tap do \|blob\|

		values += [STDOUT, StringIO.new, ActionDispatch::Http::UploadedFile.new(tempfile: __FILE__),
		values += [$stdout, StringIO.new, ActionDispatch::Http::UploadedFile.new(tempfile: __FILE__),

Introduce new cops from newer rubocop #39527

Introduce new cops from newer rubocop #39527

Uh oh!

Conversation

fatkodima commented Jun 3, 2020

Uh oh!

rails-bot bot commented Sep 1, 2020

Uh oh!

KapilSachdev commented Sep 5, 2020

Uh oh!

fatkodima commented Sep 5, 2020

Uh oh!

KapilSachdev commented Sep 5, 2020

Uh oh!

fatkodima commented Sep 5, 2020

Uh oh!

jonathanhefner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatkodima Sep 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

composerinteralia commented Sep 6, 2020

Uh oh!

fatkodima commented Sep 6, 2020

Uh oh!

jonathanhefner commented Sep 6, 2020

Uh oh!

fatkodima commented Sep 6, 2020

Uh oh!

kamipo commented Sep 6, 2020

Uh oh!

fatkodima commented Sep 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanhefner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanhefner commented Sep 6, 2020

Uh oh!

kamipo commented Sep 6, 2020

Uh oh!

fatkodima commented Sep 7, 2020

Uh oh!

KapilSachdev commented Sep 7, 2020

Uh oh!

fatkodima Sep 6, 2020 •

edited

Loading