artsy / README

:wave: - The documentation for being an Artsy Engineer
Creative Commons Attribution 4.0 International
1.1k stars 120 forks source link

Add RFC for standardrb #531

Closed jonallured closed 7 months ago

jonallured commented 7 months ago

Adds an RFC for switching to standardrb - see the markdown file that this PR adds for the details!

damassi commented 7 months ago

Big thumbs up here! Right now every project has a slightly different setup, which trips folks up and is largely unnecessary. And the blog post about the speed in which Rubocop executes with the LSP would effectively solve my issue with it, which was the ancient-feeling performance.

The best thing about Prettier is not having to think about anything at all. I love that this approach is being applied here, but with a tool that can do more.

Additionally, in terms of formatting, prettier-ruby never seems to have caught on. VSCode integration is broken, and the latest versions require that we install some additional libraries. I like the idea of removing it where we can in favor of this, and simplifying the tool stack.

joeyAghion commented 7 months ago

Removing extensive rubocop config and variation sounds great to me. (More detailed insight would depend on how that goes, and especially what the transition looks like.)

Our tech radar can help here, specifically these steps for trial-ing and eventually adopting a new tool. This PR should at least add standardrb to the "trial" stage and specify the projects (Exchange) and timeline for making an adopt/hold decision.

damassi commented 7 months ago

@jonallured - Q: Rather than Exchange, thoughts on adding this to volt as the trial app? We're doing a lot of work in Volt lately, and that will put you and @jpotts244 very close to its behavior.

Mitch-Henson commented 7 months ago

Totally onboard with the idea and that we continue to work on standardisation between repositories where possible. It has already been mentioned here, but I think that we should actually implement it in a branch for one repo (volt might be a good contender, but we are also moving away to volt V2 so maybe not) to see it working in action before we commit to it completely.

anandaroop commented 7 months ago

Love it!

In case this helps anyone make their decision, this is the basic "unconfigurable configuration" —

https://github.com/standardrb/standard/blob/main/config/base.yml

(You had me at EnforcedStyle: double_quotes)

jonallured commented 7 months ago

Ok! Sounds like mostly folks are on board with this proposal so that's great. I agreed that it was a good idea to do a proof of concept so I took some time today to run through the process on Exchange - here's the PR:

https://github.com/artsy/exchange/pull/2005/

I'm glad you suggested it @leamotta! I think my biggest take-away is that the change from single quote to double quote means the diff is enormous and very hard to review. I think there's a better way!

What I think we should do is break this process into two PRs. The first PR should update the project's RuboCop config such that it enforces double quotes. Once that's lands then actually switching to Standard Ruby will be such a smaller change that it'll be much easier to review for actual substantive or potentially breaking changes. So yeah, the second PR would make that change with the quotes already dealt with and could speed up getting through review.

Any other thoughts based on this proof of concept?

jonallured commented 7 months ago

And another example PR: https://github.com/artsy/impulse/pull/1113/ this time exploring separating out the part where we switch to double quotes and comply with the hash white space rule. When that's a separate commit it's much easier to actually see what Standard Ruby is changing and what could be a breaking change. You do have to review the PR commit-by-commit but that seems fine.

jonallured commented 7 months ago

Update: I've tried out the Standard Ruby plugin for Rails here:

https://github.com/artsy/exchange/pull/2020

It did not go well! Then I went to the project to see if there's been any discussions around the problems I was seeing and bumped into this one:

https://github.com/standardrb/standard-rails/issues/28

In which the creator of the project says that he's only used the project in earnest 3 days ago. Given my experience and that message I think it's best to skip adding this plugin to our projects and instead let it mature and come back to it later. I will update the text of this RFC to reflect this learning!

narikazu commented 7 months ago

There is a way to fetch a rubocop setting file from a remote URL and load it to a project. We standardized the rubocop setting between rails projects in the previous company. Have we considered it?

leamotta commented 7 months ago

If this effectively means we will lose linting support for Rails’ nuances (rubocop-rails) in all repos then I’m not sure I can give my 👍 when on the other side the benefit is only standarization, which we can still achieve through other means (like @narikazu's suggestion combined with more responsible dependency management across repos)

Totally non-blocking though, if folks feel strongly about going in this direction, I'm happy to help out with the next steps, alignment is important! Thank you @jonallured for all the effort involved in the work of this RFC 🙌

jonallured commented 7 months ago

@narikazu interesting! I was not aware that RuboCop had a feature like this. I see how this could solve the problem of consolidating the settings between projects. But it would require establishing a shared set of rules which is something Standard Ruby has already done. I believe that makes switching to Standard Ruby a more attractive option because establishing the rules is the hard part!


If this effectively means we will lose linting support for Rails’ nuances (rubocop-rails) in all repos then I’m not sure I can give my 👍

@leamotta this is a totally valid criticism! My hope is that we can adopt Standard Ruby's plugin for Rails once it's more mature and we'll be back to having Rails-specific linting. I will note that it's an open source project and there's nothing stopping us from providing feedback that gets it to a point where we actually want to use it! For instance, I considered opening an issue about the Rails/SaveBang cop - I'll work on that later today!

jonallured commented 7 months ago

Update: https://github.com/standardrb/standard-rails/issues/30