Fix autocorrect Rails Cops 2 #11337

macanudo527 · 2023-08-04T07:35:58Z

What? Why?

Part of Fix Rubocop issues with safe auto-correct #11253

This fixes 4 more rubocop rules that are violated in our codebase. It was generated automatically with the rubocop-autocorrect.sh script.

What should we test?

No testing is required since these are safe corrections

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

rioug · 2023-08-07T01:36:59Z

There is a failing spec which looks related, moving to In Dev

macanudo527 · 2023-08-07T23:39:12Z

@rioug This should be okay now. I fixed the previous failing spec. I think the current one is a flakey test.

rioug · 2023-08-08T00:15:41Z

spec/models/content_configuration_spec.rb

@@ -12,7 +12,7 @@
    end

    def image_exist?(default_url)
-      File.exist?(Rails.public_path.join(default_url).to_s)
+      File.exist?(Rails.public_path.to_s + default_url)


I don't think this is necessary, they are more or less equivalent, but the join version is better as it will handle properly the use of / or \ in path.

They generate two different URLs causing the test to fail. The original line generates the url to access it via web I believe. The corrected version gives you the local path.

Ahh, default_url is being interpreted as an absolute URL because it has a forward slash at the beginning of it.

EDIT: All of the URLs are hard-coded with forward slashes in content_configuration.rb. I'm not sure how to make these safe.

To be compatible, these should probably be Pathname, but that might break things elsewhere?

What about this :

Suggested change

File.exist?(Rails.public_path.to_s + default_url)

File.exist?(File.join(Rails.public_path, default_url))

Rspec passes, but Rubocop still complains:

spec/models/content_configuration_spec.rb:15:19: C: [Correctable] Rails/RootPathnameMethods: Rails.public_path is a Pathname so you can just append #join. File.exist?(File.join(Rails.public_path, default_url)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1484 files inspected, 1 offense detected, 1 offense autocorrectable

This will pass both Rspec and Rubocop:

Rails.public_path.join(*default_url.split("/")).exist?

I feel it is a little acrobatic, but I feel like it is the safest without modifying the hard-coded URLs or just ignoring this one line.

That's a tricky one, it comes down to File.join and and Pathname#join not behaving the same when there is a leading "/". Let's go with your last solution, as it behaves the same has the original code, ie having a leading "/" or not would return the same file path. Maybe just add a comment to mention why we are doing this.

Inspecting 1484 files .............................................................................................................................................................................................................................................................................................................C.............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................C....................C.....................................................................................................................................................................................................................................................................C.........................................................................................................................................................................................................C........................................................................................................................................................... Offenses: app/models/product_import/product_importer.rb:290:35: C: [Corrected] Rails/FilePath: Prefer Rails.root.join('path/to'). return unless @file.path == Rails.root.join('tmp', 'product_import').to_s ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/tasks/karma.rake:42:5: C: [Corrected] Rails/FilePath: Prefer Rails.root.join('path/to'). "#{Rails.root.join(I18n::JS::DEFAULT_EXPORT_DIR_PATH)}/translations.js" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/tasks/karma.rake:42:5: C: [Correctable] Style/RedundantInterpolation: Prefer to_s over string interpolation. "#{Rails.root.join(I18n::JS::DEFAULT_EXPORT_DIR_PATH, 'translations.js')}" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/tasks/karma.rake:42:59: C: [Corrected] Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations. "#{Rails.root.join(I18n::JS::DEFAULT_EXPORT_DIR_PATH, "translations.js")}" ^^^^^^^^^^^^^^^^^ spec/base_spec_helper.rb:58:25: C: [Corrected] Rails/FilePath: Prefer Rails.root.join('path/to'). config.fixture_path = "#{::Rails.root}/spec/fixtures" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/base_spec_helper.rb:58:25: C: [Correctable] Style/RedundantInterpolation: Prefer to_s over string interpolation. config.fixture_path = "#{Rails.root.join('spec/fixtures')}" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/base_spec_helper.rb:58:44: C: [Corrected] Style/StringLiteralsInInterpolation: Prefer single-quoted strings inside interpolations. config.fixture_path = "#{Rails.root.join("spec/fixtures")}" ^^^^^^^^^^^^^^^ spec/models/content_configuration_spec.rb:15:19: C: [Corrected] Rails/FilePath: Prefer Rails.root.join('path/to').to_s. File.exist?(File.join(Rails.root, 'public', default_url)) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/models/content_configuration_spec.rb:15:19: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. File.exist?(Rails.root.join('public', default_url).to_s) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/support/downloads_helper.rb:7:5: C: [Corrected] Rails/FilePath: Prefer Rails.root.join('path/to'). Rails.root.join("tmp", "capybara") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1484 files inspected, 10 offenses detected, 8 offenses corrected, 2 more offenses can be corrected with `rubocop -A`

Inspecting 1484 files .......................................C.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................... Offenses: app/controllers/admin/proxy_orders_controller.rb:16:35: C: [Corrected] Rails/I18nLazyLookup: Use "lazy" lookup for the text used in controllers. render json: { errors: [t('admin.proxy_orders.cancel.could_not_cancel_the_order')] }, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ app/controllers/admin/proxy_orders_controller.rb:25:35: C: [Corrected] Rails/I18nLazyLookup: Use "lazy" lookup for the text used in controllers. render json: { errors: [t('admin.proxy_orders.resume.could_not_resume_the_order')] }, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1484 files inspected, 2 offenses detected, 2 offenses corrected

Inspecting 1484 files .............................................................................................C........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................C.........................................................................................C..........C..........................................................................................................................................................................................................................................................................................................C....................................................................................................................................................................C...................C.............................................................C.C.......C.................... Offenses: app/controllers/concerns/request_timeouts.rb:19:22: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. file: Rails.root.join("public/500.html"), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/spree/core/controller_helpers/common.rb:45:30: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. file: Rails.root.join("public/404.html"), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/controllers/api/v0/product_images_controller_spec.rb:13:22: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. let(:pdf_path) { Rails.root.join("public/Terms-of-service.pdf") } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/controllers/api/v0/terms_and_conditions_controller_spec.rb:15:31: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. let(:terms_file_path) { Rails.root.join("public/Terms-of-service.pdf") } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/models/terms_of_service_file_spec.rb:6:25: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. let(:pdf) { File.open(Rails.root.join("public/Terms-of-service.pdf")) } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/admin/bulk_product_update_spec.rb:907:37: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. attach_file 'image-upload', Rails.root.join("public/500.jpg"), visible: false ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/admin/enterprises/terms_and_conditions_spec.rb:27:30: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. let(:original_terms) { Rails.root.join("public/Terms-of-service.pdf") } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/admin/enterprises/terms_and_conditions_spec.rb:28:29: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. let(:updated_terms) { Rails.root.join("public/Terms-of-ServiceUK.pdf") } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/consumer/shopping/checkout_spec.rb:94:9: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. Rails.root.join("public/Terms-of-service.pdf"), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/consumer/shopping/embedded_groups_spec.rb:18:9: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. Rails.root.join("public/embedded-group-preview.html") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/consumer/shopping/embedded_groups_spec.rb:26:9: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. Rails.root.join("public/embedded-group-preview.html") ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/consumer/split_checkout_spec.rb:986:35: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. let(:system_terms_path) { Rails.root.join("public/Terms-of-service.pdf") } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/system/consumer/split_checkout_spec.rb:987:33: C: [Corrected] Rails/RootPublicPath: Use Rails.public_path. let(:shop_terms_path) { Rails.root.join("public/Terms-of-ServiceUK.pdf") } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1484 files inspected, 13 offenses detected, 13 offenses corrected

Inspecting 1484 files ........................................................................................................................................................................................................................................................C.........................................................................................................................................CC...........................................................................................................................................................................................................................................................................................................................................................................................C.......................................................................C.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................... Offenses: app/models/content_configuration.rb:74:48: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. preference :footer_links_md, :text, default: <<-EOS.strip_heredoc ^^^^^^^^^^^^^^^^^^^^ app/queries/customers_with_balance.rb:23:5: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-SQL.strip_heredoc ^^^^^^^^^^^^^^^^^^^^ app/queries/outstanding_balance.rb:32:5: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-SQL.strip_heredoc ^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:71:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:84:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:96:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:109:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:119:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:135:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:151:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:161:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:168:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:182:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:192:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:208:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:218:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:228:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:235:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:253:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:274:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:289:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:310:13: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/reporting/reports/enterprise_fee_summary/scope.rb:369:15: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. <<-JOIN_STRING.strip_heredoc ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ lib/tasks/data/truncate_data.rake:35:17: C: [Corrected] Rails/StripHeredoc: Use squiggly heredoc (<<~) instead of strip_heredoc. message = <<-MSG.strip_heredoc ^^^^^^^^^^^^^^^^^^^^ lib/tasks/data/truncate_data.rake:36:1: C: [Corrected] Layout/HeredocIndentation: Use 2 spaces for indentation in a heredoc. \n ... ^^^^^^^^ 1484 files inspected, 25 offenses detected, 25 offenses corrected

rioug

Thanks @macanudo527 Looks good now 👍

mkllnk

Nice work. Thank you.

mkllnk · 2023-08-16T23:25:18Z

.rubocop_todo.yml

+# Offense count: 2
+# This cop supports unsafe autocorrection (--autocorrect-all).
+Style/RedundantInterpolation:
+  Exclude:
+    - 'lib/tasks/karma.rake'
+    - 'spec/base_spec_helper.rb'


This should have been addressed in the previous commit which introduced these.

mkllnk · 2023-08-16T23:50:26Z

spec/models/content_configuration_spec.rb

@@ -12,7 +12,7 @@
    end

    def image_exist?(default_url)
-      File.exist?(File.join(Rails.root, 'public', default_url))
+      Rails.public_path.join(*default_url.split("/")).exist?


I got another version to tell Rubocop that we are not interested in Pathname logic:

Suggested change

Rails.public_path.join(*default_url.split("/")).exist?

File.exist?(File.join(Rails.public_path.to_s, default_url))

The autocorrection also looks like a bug in rubocop-rails. It has been reported before:

Rails/RootPathnameMethods: different behaviour between File.join and Rails.root.join rubocop/rubocop-rails#911

There was a fix applied to one cop but this bug seems to be in another cop. This is not a safe correction. I opened an issue:

FilePath safe autocorrect breaks code rubocop/rubocop-rails#1071

mkllnk · 2023-08-17T00:39:24Z

No testing is required since these are safe corrections

This proofed to be wrong. Never trust Rubocop. 😉

The rest should be safe though. Risk is low, test coverage quite good. So I'll merge because of the risk of conflicts.

macanudo527 force-pushed the fix_rubocop_6 branch from 8414bf2 to 934226e Compare August 4, 2023 13:48

macanudo527 force-pushed the fix_rubocop_6 branch 2 times, most recently from 43cba01 to 937a6a0 Compare August 7, 2023 13:56

rioug requested changes Aug 8, 2023

View reviewed changes

macanudo527 force-pushed the fix_rubocop_6 branch 2 times, most recently from 515d7ad to af4ff2c Compare August 14, 2023 08:26

macanudo527 added 6 commits August 14, 2023 17:29

Regenerate Rubocop's TODO file

c367286

Reword image_exists? to use Rails.public_path

ec49065

macanudo527 force-pushed the fix_rubocop_6 branch from af4ff2c to ec49065 Compare August 14, 2023 08:29

rioug approved these changes Aug 14, 2023

View reviewed changes

mkllnk approved these changes Aug 17, 2023

View reviewed changes

mkllnk merged commit 27dcad2 into openfoodfoundation:master Aug 17, 2023

macanudo527 deleted the fix_rubocop_6 branch August 17, 2023 04:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix autocorrect Rails Cops 2 #11337

Fix autocorrect Rails Cops 2 #11337

macanudo527 commented Aug 4, 2023

rioug commented Aug 7, 2023

macanudo527 commented Aug 7, 2023

rioug Aug 8, 2023

macanudo527 Aug 8, 2023

macanudo527 Aug 8, 2023 •

edited

Loading

rioug Aug 8, 2023

macanudo527 Aug 8, 2023

macanudo527 Aug 8, 2023

rioug Aug 11, 2023

rioug left a comment

mkllnk left a comment

mkllnk Aug 16, 2023

mkllnk Aug 16, 2023

mkllnk commented Aug 17, 2023

	File.exist?(Rails.public_path.to_s + default_url)
	File.exist?(File.join(Rails.public_path, default_url))

	Rails.public_path.join(*default_url.split("/")).exist?
	File.exist?(File.join(Rails.public_path.to_s, default_url))

Fix autocorrect Rails Cops 2 #11337

Fix autocorrect Rails Cops 2 #11337

Conversation

macanudo527 commented Aug 4, 2023

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

Documentation updates

rioug commented Aug 7, 2023

macanudo527 commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macanudo527 Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkllnk commented Aug 17, 2023

macanudo527 Aug 8, 2023 •

edited

Loading