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

Use Standard Ruby instead of Rubocop #6132

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Feb 18, 2025

Summary

This (rather large) PR changes our linter from rubocop to standardrb.

The advantages are

  • Standardrb does not change quite as annoyingly often
  • Standardrb promises to stop us arguing about code style

See https://github.com/standardrb/standard?tab=readme-ov-file for more arguments.

I will only rebase this once if I have two approvals. It's rather cumbersome because of the large amount of change.

The two commits that introduce all the fixes are exempted from git blame through the .git-blame-ignore-revs file.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

These rules make sense for an Application, but in the case of Solidus
they do not make sense. We really want to inherit from Rails' base
classes, and we happen to have a configurable user class.
I've not committed things that look odd.
These problems can't be fixed automatically and must be fixed by
investing time.
Also don't upload data in GH action, standard does not seem to support
it.
@mamhoff mamhoff requested a review from a team as a code owner February 18, 2025 12:57
@github-actions github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus_sample Changes to the solidus_sample gem changelog:solidus Changes to the solidus meta-gem changelog:solidus_admin changelog:solidus_legacy_promotions Changes to the solidus_legacy_promotions gem changelog:solidus_promotions Changes to the solidus_promotions gem labels Feb 18, 2025
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I like this very much. We are using standard in all our client shops with huge success. But I would like to configure rubocop with standard rules instead.

Gemfile Outdated
gem 'rubocop', '~> 1', require: false
gem 'rubocop-performance', '~> 1.4', require: false
gem 'rubocop-rails', '~> 2.9', require: false
gem 'standard', require: false
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep rubocop as dependency and configure it to use standard. See https://github.com/standardrb/standard?tab=readme-ov-file#running-standards-rules-via-rubocop

That way Tools like VSCode extension ruby-lsp and other Ruby IDEs still can use their rubocop integrations

Copy link
Member

@jarednorman jarednorman Feb 18, 2025

Choose a reason for hiding this comment

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

ruby-lsp can be configured to use standard as the formatter, which I find preferable than using it for diagnostics (as rubocop is normally used). There's no real use in showing cop violations if you're using that kind of set up, because they will get automatically fixed by format-on-save.

Copy link
Member

Choose a reason for hiding this comment

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

ruby-lsp can be configured to use standard as the formatter, which I find preferable than using it for diagnostics (as rubocop is normally used)

There are many tools that only have support for Rubocop and since Standard builds on top of Rubocop it does not make any difference other than that we also support tools that only support Rubocop. I would prefer that we do not require people to have a IDE that supports Standard as well.

There's no real use in showing cop violations if you're using that kind of set up, because they will get automatically fixed by format-on-save.

Not sure what you mean with that. Rubocop is configured as default formatter and "format on save" is an independent setting in VSCode that just uses the formatter configured. This seems tangential to me. Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with your approach if you think it will generally be easier for people.

I don't know about VSCode, but when I set up projects these days (Ruby or otherwise), I try to set them up to make full use of the language server for that language. This way anyone with an editor that supports language servers gets the best possible experience.

When I set up Ruby projects, I configure ruby-lsp to use Standard as a formatter, not to provide linting errors in the editor. This leaves me the option of using Rubocop for linting if I so choose, without causing Standard's (autofixable) formatting hints to show up as linting errors in my editor, which isn't helpful, since they will be fixed automatically (assuming my editor is configured to use the language server's formatter for format-on-save, which it should be.)

Basically, formatting and linting are not the same thing, so I try to keep them separate. But, like I said, if you think configuring Standard through Rubocop will provide the best experience for most people, I won't fight you on that. I don't know how everyone else works.

@tvdeyen tvdeyen marked this pull request as draft February 18, 2025 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_admin changelog:solidus_api Changes to the solidus_api gem changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem changelog:solidus_legacy_promotions Changes to the solidus_legacy_promotions gem changelog:solidus_promotions Changes to the solidus_promotions gem changelog:solidus_sample Changes to the solidus_sample gem changelog:solidus Changes to the solidus meta-gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants