Replace rubocop with standard #3951

tejasbubane · 2021-02-21T19:36:30Z

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

houndci-bot · 2021-02-21T19:36:44Z

Gemfile

@@ -83,4 +81,4 @@ gem 'rspec_junit_formatter', require: false, group: :ci
 gem 'yard', require: false, group: :docs

 custom_gemfile = File.expand_path('Gemfile-custom', __dir__)
-eval File.read(custom_gemfile), nil, custom_gemfile, 0 if File.exist?(custom_gemfile)
+eval File.read(custom_gemfile), nil, custom_gemfile, 0 if File.exist?(custom_gemfile) # rubocop:disable Security/Eval


Lint/UnneededCopDisableDirective: Unnecessary disabling of Security/Eval.

houndci-bot · 2021-02-21T19:36:44Z

backend/app/controllers/spree/admin/orders_controller.rb

                                         ""
                                       end
        end

        if params[:q][:created_at_lt].present?
          params[:q][:created_at_lt] = begin
                                         Time.zone.parse(params[:q][:created_at_lt]).end_of_day
-                                       rescue StandardError
+                                       rescue


Style/RescueStandardError: Avoid rescuing without specifying an error class.

houndci-bot · 2021-02-21T19:36:44Z

backend/app/controllers/spree/admin/orders_controller.rb

@@ -28,15 +28,15 @@ def index
        if params[:q][:created_at_gt].present?
          params[:q][:created_at_gt] = begin
                                         Time.zone.parse(params[:q][:created_at_gt]).beginning_of_day
-                                       rescue StandardError
+                                       rescue


Style/RescueStandardError: Avoid rescuing without specifying an error class.

houndci-bot · 2021-02-21T19:36:45Z

core/app/helpers/spree/base_helper.rb

@@ -100,7 +100,7 @@ def taxons_tree(root_taxon, current_taxon, max_level = 1)
      return '' if max_level < 1 || root_taxon.children.empty?
      content_tag :ul, class: 'taxons-list' do
        taxons = root_taxon.children.map do |taxon|
-          css_class = (current_taxon && current_taxon.self_and_ancestors.include?(taxon)) ? 'current' : nil
+          css_class = current_taxon && current_taxon.self_and_ancestors.include?(taxon) ? 'current' : nil


Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.

houndci-bot · 2021-02-21T19:36:45Z

core/app/models/spree/address.rb

@@ -87,7 +87,7 @@ def to_s
    # @note This compares the addresses based on only the fields that make up
    #   the logical "address" and excludes the database specific fields (id, created_at, updated_at).
    # @return [Boolean] true if the two addresses have the same address fields
-    def ==(other_address)
+    def ==(other_address) # rubocop:disable Naming/BinaryOperatorParameterName


Lint/UnneededCopDisableDirective: Unnecessary disabling of Naming/BinaryOperatorParameterName.

houndci-bot · 2021-02-21T19:36:45Z

core/lib/spree/testing_support/factory_bot.rb

@@ -37,7 +37,7 @@ def self.deprecate_cherry_picking
        Spree::Deprecation.warn(
          "Please do not cherry-pick factories, this is not well supported by FactoryBot, " \
          'follow the changelog instructions on how to migrate your current setup.',
-          callsites[index..-1]
+          callsites[index..]


Lint/Syntax: unexpected token tRBRACK

houndci-bot · 2021-02-21T19:36:45Z

core/app/models/spree/unit_cancel.rb

@@ -48,6 +48,6 @@ def weighted_line_item_amount(line_item)
  end

  def quantity_of_line_item(line_item)
-    BigDecimal(line_item.inventory_units.not_canceled.reject(&:original_return_item).size)
+    BigDecimal(line_item.inventory_units.not_canceled.reject(&:original_return_item).size) # rubocop:disable Performance/Count


Lint/UnneededCopDisableDirective: Unnecessary disabling of Performance/Count.

houndci-bot · 2021-02-21T19:36:46Z

core/spec/lib/spree/money_spec.rb

@@ -246,9 +246,11 @@
    let(:usd_30) { Spree::Money.new(30, currency: "USD") }

    it "compares the two amounts" do
+      # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands


Lint/UnneededCopDisableDirective: Unnecessary disabling of Lint/BinaryOperatorWithIdenticalOperands (unknown cop).

houndci-bot · 2021-02-21T19:36:46Z

core/app/models/spree/order.rb

@@ -452,7 +453,7 @@ def empty!
    def coupon_code=(code)
      @coupon_code = begin
                       code.strip.downcase
-                     rescue StandardError
+                     rescue


Style/RescueStandardError: Avoid rescuing without specifying an error class.

houndci-bot · 2021-02-21T19:36:46Z

guides/lib/custom_markdown_renderer.rb

@@ -7,7 +7,7 @@ class CustomMarkdownRenderer < Redcarpet::Render::HTML

  def block_code(code, language)
    path = code.lines.first[/^#\s(\S*)$/, 1]
-    code = code.lines[1..-1].join if path
+    code = code.lines[1..].join if path


Lint/Syntax: unexpected token tRBRACK

jarednorman · 2021-02-25T01:11:52Z

While I don't want to be the one pushing for a ton of formatting churn, I'm also concerned we're not going to get a huge amount of value from using the tool if it's configured such that so much of its functionality is disabled. Might need further consideration with @solidusio/core-team.

tejasbubane · 2021-02-25T08:14:08Z

I'm also concerned we're not going to get a huge amount of value from using the tool if it's configured such that so much of its functionality is disabled.

I agree and I realized this after completing the task. The whole point of using standard is to not have to configure anything. This more or less defeats the purpose.

aldesantis · 2021-02-25T08:27:29Z

I agree. If we move to Standard, we should probably just fix all the offenses. We're basically going to touch every single file in the codebase, but at least we'll only have to do it once.

What does the rest of @solidusio/core-team think?

tvdeyen · 2021-02-25T08:29:54Z

I like the idea of opinionated, non-config code formatters like rufo or standard. The thing is that every contributor would need to use it and we would still need to lint the code with rubocop, otherwise the rules wouldnt be enforced. Its a tricky situation. Unfortunately the code formatting is not build into the language.

tejasbubane · 2021-02-25T08:40:42Z

@tvdeyen

we would still need to lint the code with rubocop, otherwise the rules wouldnt be enforced

you mean lint with standard?

Its a tricky situation. Unfortunately the code formatting is not build into the language.

standard or rubocop is almost same as built-in formatters like gofmt or rustfmt right? Users need to install it, here they do it with bundle install. CI needs to run them to check for formatting.

One issue I see though is that configuration is not standardized across all users of the language - hence some contributors might find themselves manually fixing some weird issues (in case autocorrect does not work in those cases).

tvdeyen · 2021-02-25T08:47:16Z

@tvdeyen

we would still need to lint the code with rubocop, otherwise the rules wouldnt be enforced

you mean lint with standard?

Standard uses rubocop, so yes. If we go with the default rubocop rules of standard (what I encourage us to do, otherwise it would not make sense to use it at all), we would use standard to lint as well. 👍🏻

Its a tricky situation. Unfortunately the code formatting is not build into the language.

standard or rubocop is almost same as built-in formatters like gofmt or rustfmt right? Users need to install it, here they do it with bundle install. CI needs to run them to check for formatting. Or am I missing something?

Unfortuantely its not. The people would still need to install it and configure their editor to "format on safe" with the tool that we force on them. They might have it disabled or even worse configured differently.

The idea of automatic code formatters is great, but unless build into the language from the beginning, you are screwed.

tvdeyen · 2021-02-25T08:54:21Z

Take the double-quotes rule as example. Prettierrb has it changed from the Prettierjs default to false (to enforce single quotes), just because it is more common in Ruby code to use single quotes. This violates the whole idea of prettier.

The idea was to not fight over code formatting rules anymore, and use a tool for that. Now people fight over the rules. Its ridiculous.

Imagine the following situation:

A dev has configured their editor to use prettierrb to format .rb files on safe, now wants to contribute to solidus and is enforced to use double-quotes. Giving them a bad time fighting its own editor just to fulfill our rules. Not very welcoming.

The only unobtrusive thing I could imagine is to automatically run standard --fix on the CI as an after commit hook or something.

jarednorman · 2021-02-25T17:29:04Z

On our projects we usually use our autoformatters with a tool like lint-staged and husky to provide automatic and git add -p-friendly commit hooks that autoformat everything seemlessly. That's the ideal, I think.

bogdanvlviv · 2021-05-12T13:36:57Z

It is lovely to see how we use rubocop in rails.

We disable all default cops https://github.com/rails/rails/blob/main/.rubocop.yml#L10 and only enable those ones that reflect our style guide in the project.
The rubocop.yml file read great https://github.com/rails/rails/blob/main/.rubocop.yml#L10. By reading it we can learn about the style Rails repo uses.

One example of how rubocop gives a hand while improving the codebase:

We noticed that we dropped support for < Ruby 2.7 so that we could use filter_map gem to speed up some lines of the codebase. rails/rails#42053. With rubocop, we were able to catch all the places we could improve by filter_map and make sure that we would use this method where possible instead of map+compact, or something in the future, and never return to this topic again.
rails/rails@bbbc861.
We might use rubocop in the similar way in solidusio/solidus. WDYT?

P. S.There are more interesting samples https://github.com/rails/rails/pulls?q=is%3Apr+rubocop+is%3Aclosed+author%3Akamipo

kennyadsl · 2021-05-13T06:39:57Z

@bogdanvlviv I really like the approach proposed. We can gradually introduce rules based on the feedback we provide in PR reviews and use Rubocop as a tool to refactor code over time. 👍

jarednorman · 2021-05-13T16:26:15Z

I more in favour of the set it and forget approach of Standard. Continuing to maintain a rubocop configuration, especially across the extension ecosystem (though I know we could rely on solidus_dev_support) doesn't seem like a good use of maintenance effort to me. That said, if there were some rules we wanted to enable on top of the Standard defaults, like ones that deter people from doing insecure things, I'm open it.

spaghetticode · 2021-05-14T09:36:21Z

I'm not really in favor of basically touching every single file in the codebase for cosmetic reasons (or at least debatable reasons).

While I personally like standard more than rubocop (but in general I'm not a fan of code linters, at least in the ruby ecosystem), I think the change would be pretty invasive and pollute the git history. And there's no guarantee it would be final, in a few years we may be still here evaluating the new best linter and debating if we should change all the files again.

jarednorman · 2021-05-14T17:44:36Z

The concern about touching every file is fair; it's not ideal, but I don't consider it a dealbreaker if what we get in exchange for a one-off formatting commit is to stop worrying about this style formatting stuff.

I don't agree with your concern about further churn down the road. The problem with maintaining a Rubocop config (across the whole Solidus ecosystem) is that Rubocop continues to add, remove and change rules, which requires us to evaluate them and decide whether to integrate with (and at least keep up with changes to rules we use.) Using Rubocop is like agreeing to debate style forever.

The whole point of Standard (especially now that it has hit v1.0) is that they've done the work to debate the preferences of the Ruby community and have adopted a reasonably conservative set of sensible defaults. If we adopt it, we agree to basically never do this again barring some kind of catastrophe, otherwise I agree it's not worth changing.

While I don't like formatting churn standard'll cause, only one of these tools purports to offer the opportunity to just use the tool's default settings and never worry about this again.

jarednorman · 2021-05-14T17:45:42Z

And to be clear, I'm still not 100% sure we should do this, I just really don't want to maintain a Rubocop config. It just seems like a waste of time when there's a "set it and forget it" alternative.

tvdeyen · 2021-05-14T18:55:17Z

@jarednorman 💯 agree. Well said.

aldesantis · 2021-05-15T07:30:16Z

I also agree with @jarednorman. Going with Standard is not just another coding style change - it means not having to worry about coding style ever again, which is a very alluring idea.

As for touching every file and polluting the Git history, I don't see much of a problem with that, as long as it's a one-off thing.

jarednorman · 2022-06-16T22:58:48Z

@tejasbubane Thanks for your patience with our back and forth on this issue.

After a Core Team discussion we've decided that for now this change would be a bit too disruptive. It would mean everyone would have to rebase all their open PRs, we'd have to update the whole extension ecosystem to match, and it would generate a lot of noise in the history of all the files in the project.

I think we'd all prefer to be using Standard (and without a bunch of rules disabled), but the benefits don't outweigh the disruption. We think there might be a moment down the road where some other large, disruptive change gets made where we'd sneak this one in too just to get it all over with, so we'll cross that bridge when we come to it.

tejasbubane added 2 commits February 22, 2021 00:59

Replace rubocop with standard

5995f34

https://github.com/testdouble/standard

Add standard config and fix all errors

3d1256f

houndci-bot reviewed Feb 21, 2021

View reviewed changes

tejasbubane force-pushed the standard branch from e833ab7 to dff7bce Compare February 21, 2021 19:39

Add lint job to CircleCI

dff7bce

tejasbubane mentioned this pull request Feb 21, 2021

Update Rubocop rules #3872

Open

jarednorman closed this Jun 16, 2022

tejasbubane deleted the standard branch June 17, 2022 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace rubocop with standard #3951

Replace rubocop with standard #3951

tejasbubane commented Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

houndci-bot Feb 21, 2021

jarednorman commented Feb 25, 2021

tejasbubane commented Feb 25, 2021

aldesantis commented Feb 25, 2021

tvdeyen commented Feb 25, 2021

tejasbubane commented Feb 25, 2021 •

edited

Loading

tvdeyen commented Feb 25, 2021

tvdeyen commented Feb 25, 2021 •

edited

Loading

jarednorman commented Feb 25, 2021

bogdanvlviv commented May 12, 2021 •

edited

Loading

kennyadsl commented May 13, 2021

jarednorman commented May 13, 2021

spaghetticode commented May 14, 2021

jarednorman commented May 14, 2021

jarednorman commented May 14, 2021

tvdeyen commented May 14, 2021

aldesantis commented May 15, 2021 •

edited

Loading

jarednorman commented Jun 16, 2022

Replace rubocop with standard #3951

Replace rubocop with standard #3951

Conversation

tejasbubane commented Feb 21, 2021

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

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

jarednorman commented Feb 25, 2021

tejasbubane commented Feb 25, 2021

aldesantis commented Feb 25, 2021

tvdeyen commented Feb 25, 2021

tejasbubane commented Feb 25, 2021 • edited Loading

tvdeyen commented Feb 25, 2021

tvdeyen commented Feb 25, 2021 • edited Loading

jarednorman commented Feb 25, 2021

bogdanvlviv commented May 12, 2021 • edited Loading

kennyadsl commented May 13, 2021

jarednorman commented May 13, 2021

spaghetticode commented May 14, 2021

jarednorman commented May 14, 2021

jarednorman commented May 14, 2021

tvdeyen commented May 14, 2021

aldesantis commented May 15, 2021 • edited Loading

jarednorman commented Jun 16, 2022

tejasbubane commented Feb 25, 2021 •

edited

Loading

tvdeyen commented Feb 25, 2021 •

edited

Loading

bogdanvlviv commented May 12, 2021 •

edited

Loading

aldesantis commented May 15, 2021 •

edited

Loading