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

Apply all standard fixes #926

Closed
wants to merge 2 commits into from

Conversation

czj
Copy link
Contributor

@czj czj commented Jun 21, 2023

Following #925 this PR shows what default standardrb configuration would change.

@minad
Copy link
Member

minad commented Jun 21, 2023

Thanks for showing these changes. Please take it slowly. These are far too many pointless syntactic changes to apply them just like this. One problem is that these changes make it much harder to use git blame, something which I consider important when investigating and fixing actual bugs. I didn't communicate clearly before. By "standard" I assumed you meant the Rubocop default settings, while in fact you had already linked to the Standard gem. But it is likely that we won't be happy even with the Rubocop default settings, since they may also result in many irrelevant changes. I suggest that we try to use Rubocop and at first only enable rules which are mostly already satisfied. Then inconsistencies can be fixed.

@minad minad closed this Jun 21, 2023
@minad minad mentioned this pull request Jun 21, 2023
@dentarg
Copy link
Contributor

dentarg commented Jun 21, 2023

Re: git blame, it is possible to make git ignore certain revisions and GitHub has built in support for it: https://github.com/orgs/community/discussions/5033

@minad
Copy link
Member

minad commented Jun 21, 2023

@dentarg I am aware. It is still a complication. To be clear, I have to investigate first which approach will be best for Slim regarding linting. Generally using a standardized coding style is a good idea, but these StandardRB suggestions here go too far with superfluous changes like changing quotes. Otoh I would also like to avoid an unnecessary bikeshedding discussion. Please follow #927.

@czj czj deleted the apply-standardrb-fixes branch June 22, 2023 08:41
@czj
Copy link
Contributor Author

czj commented Jun 22, 2023

No problem. This pull request was not meant to be merged, only to show the changes that Standard.rb would apply to this codebase.

If you want to go slow, you could add Rubocop to the project and then do one of the following:

  • Create a basic rubocop.yml and a rubocop-todo.yml that disables each cop in error, on each file, based on the codebase. This is the simplest and most progressive approach, but if you don't come back to this todo file, the benefits of linting a project that doesn't create new files often will be minimal.
  • Create a rubocop.yml with all cops disabled by default, then add those you deem relevant, one by one (small steps, when you have time). Disabling all cops by default is simpler, but you won't benefit from newly added cops. If you don't come back to the configuration often, you might miss on some useful linting.
  • Create a rubocop.yml with all cops enabled by default, then customize it to make corrections minimal and relevant to the codebase. You will benefit from newly added cops on each Rubocop release.

@minad
Copy link
Member

minad commented Jun 24, 2023

Yes, one of those approaches could work. Obviously the result should be beneficial in the end, which precludes the first approach you describe. The most useful linting rules are the performance or correctness related ones, e.g., potential type errors. Such rules would be my priority, in contrast to rules concerning syntactic details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants