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

Replace Ruby style guide with standard #606

Merged
merged 1 commit into from
Jun 12, 2020
Merged

Conversation

composerinteralia
Copy link
Contributor

@composerinteralia composerinteralia commented Jun 5, 2020

closes #605

standard is opinionated about
style, and some of those opinions are in conflict with our current style
guide.

This commit adds standard to the style guide, and removes any points
that standard already covers. I also ran standard to fix our sample.rb
to match this change.

This commit also removes .rubocop.yml. This will affect projects
which point to this file (e.g. factory_bot). They will need to either
point to a specific commit where the file still exists, or upgrade to
start using standard.

composerinteralia added a commit to thoughtbot/factory_bot that referenced this pull request Jun 5, 2020
Following an [update to the thoughtbot guide][guides PR], this commit
removes Rubocop and replaces it with [standard].

[guides PR]: thoughtbot/guides#606
[standard]: https://github.com/testdouble/standard
composerinteralia added a commit to thoughtbot/factory_bot that referenced this pull request Jun 5, 2020
Following an [update to the thoughtbot guide][guides PR], this commit
removes Rubocop and replaces it with [standard].

[guides PR]: thoughtbot/guides#606
[standard]: https://github.com/testdouble/standard
composerinteralia added a commit to thoughtbot/factory_bot_rails that referenced this pull request Jun 5, 2020
Following an [update to the thoughtbot guide][guides PR], this commit
removes Rubocop and replaces it with [standard].

[guides PR]: thoughtbot/guides#606
[standard]: https://github.com/testdouble/standard
Copy link
Contributor

@jferris jferris left a comment

Choose a reason for hiding this comment

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

✂️ ✂️ ✂️

Copy link
Contributor

@mike-burns mike-burns left a comment

Choose a reason for hiding this comment

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

Some of the items in this list were formatting related and some were based in reducing the number of trivial decisions we have to make, and I would be sad if I have to start making trivial decisions.

That said, I want to delete the styleguide, so LGTM!

style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Outdated Show resolved Hide resolved
composerinteralia added a commit to thoughtbot/factory_bot that referenced this pull request Jun 10, 2020
Following an [update to the thoughtbot guide][guides PR], this commit
removes Rubocop and replaces it with [standard].

[guides PR]: thoughtbot/guides#606
[standard]: https://github.com/testdouble/standard
composerinteralia added a commit to thoughtbot/factory_bot_rails that referenced this pull request Jun 10, 2020
Following an [update to the thoughtbot guide][guides PR], this commit
removes Rubocop and replaces it with [standard].

[guides PR]: thoughtbot/guides#606
[standard]: https://github.com/testdouble/standard
Copy link
Contributor

@georgebrock georgebrock left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

I went through and tried out all of the rules here in a project that uses the latest version of Standard, and I've flagged the ones that aren't either enforced automatically be Standard, or contradicted by Standard.

I don't feel strongly about most of the rules I've suggested keeping, but I think it would be good for maintaining clear Git history to limit the changes in this PR to only things that are replaced by Standard.

style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
style/ruby/README.md Show resolved Hide resolved
@composerinteralia
Copy link
Contributor Author

Wow! Thanks for going through all of these. It is not something I was eager to do myself.

[standard](https://github.com/testdouble/standard) is opinionated about
style, and some of those opinions are in conflict with our current style
guide.

This commit adds standard to the style guide, and removes any points
that standard already covers. I also ran standard to fix our sample.rb
to match this change.

This commit also removes .rubocop.yml. This will affect projects
which point to this file (e.g. factory_bot). They will need to either
point to a specific commit where the file still exists, or upgrade to
start using standard.
style/ruby/sample.rb Show resolved Hide resolved
style/ruby/sample.rb Show resolved Hide resolved
style/ruby/sample.rb Show resolved Hide resolved
style/ruby/sample.rb Show resolved Hide resolved
style/ruby/sample.rb Show resolved Hide resolved
@jferris
Copy link
Contributor

jferris commented Jun 12, 2020

For what it's worth, I don't feel as though any of these are worth keeping if they can't be automated. Anything that falls more in the category of a best practice can be moved to best practices. I'd rather drop the others.

I still agree with some of the ones that have been deleted, but they aren't important enough to me that I want to try and enforce them without automation. If there are any we particularly care about, we could try to get them into Standard.

@composerinteralia
Copy link
Contributor Author

For what it's worth, I don't feel as though any of these are worth keeping if they can't be automated. Anything that falls more in the category of a best practice can be moved to best practices. I'd rather drop the others.

I agree with this, but I also agree with limiting the scope of this PR. I will look into opening separate PRs to move or remove the others.

@pablobm
Copy link

pablobm commented Jun 18, 2020

I'm not 100% sure, but I think this PR may have brought unintended consequences.

If I understand correctly, Hound's configuration across thoughtbot projects uses the file style/ruby/.rubocop.yml that was deleted here. With that file missing, Hound is now applying Rubocop's default rules, which are neither like thoughtbot's old rules, nor like StandardRB's.

As a result, I'm seeing unexpected Hound comments over at Administrate.

If my assessment is correct, I guess the fix would be to tell Hound to use StandardRB? I don't know if it's possible to directly tell it to use it. Perhaps instead it can be directed to use these Rubocop config files https://github.com/testdouble/standard/tree/master/config?

@composerinteralia
Copy link
Contributor Author

@pablobm Thanks for the comment. We are moving away from Hound, but in the meantime it should probably still work as before.

I wonder if it is possible to update
https://github.com/thoughtbot/guides/blob/master/.hound.yml#L17 to be a url to an old commit with the Rubocop config file instead of a path. If not, we may need to bring back the Rubocop config for now.

composerinteralia added a commit to thoughtbot/suspenders that referenced this pull request Jun 24, 2020
https://github.com/testdouble/standard

thoughtbot is moving toward standard instead of Hound + Rubocop:
thoughtbot/guides#606
composerinteralia added a commit to thoughtbot/suspenders that referenced this pull request Jun 25, 2020
https://github.com/testdouble/standard

thoughtbot is moving toward standard instead of Hound + Rubocop:
thoughtbot/guides#606
@composerinteralia
Copy link
Contributor Author

I added back .rubocop.yml for now in 9ac4a38

composerinteralia added a commit to thoughtbot/suspenders that referenced this pull request Jul 10, 2020
https://github.com/testdouble/standard

thoughtbot is moving toward standard instead of Hound + Rubocop:
thoughtbot/guides#606
composerinteralia added a commit to thoughtbot/suspenders that referenced this pull request Jul 10, 2020
https://github.com/testdouble/standard

thoughtbot is moving toward standard instead of Hound + Rubocop:
thoughtbot/guides#606
connorchris831 pushed a commit to connorchris831/factory_bot that referenced this pull request Dec 13, 2022
Following an [update to the thoughtbot guide][guides PR], this commit
removes Rubocop and replaces it with [standard].

[guides PR]: thoughtbot/guides#606
[standard]: https://github.com/testdouble/standard
SriHarshaxi pushed a commit to SriHarshaxi/factory_bot that referenced this pull request Mar 17, 2023
Following an [update to the thoughtbot guide][guides PR], this commit
removes Rubocop and replaces it with [standard].

[guides PR]: thoughtbot/guides#606
[standard]: https://github.com/testdouble/standard
Web-Go-To added a commit to Web-Go-To/rails_suspenders that referenced this pull request Mar 23, 2023
https://github.com/testdouble/standard

thoughtbot is moving toward standard instead of Hound + Rubocop:
thoughtbot/guides#606
nickcharlton added a commit to thoughtbot/administrate that referenced this pull request Feb 2, 2024
Some time ago, we at thoughtbot stopped using our custom Rubocop rules
and replaced it with standard. This adds Standard, sets up support for
it in GitHub Actions and runs it against the code base.

https://github.com/standardrb/standard
thoughtbot/guides#606
nickcharlton added a commit to thoughtbot/administrate that referenced this pull request Feb 5, 2024
Some time ago, we at thoughtbot stopped using our custom Rubocop rules
and replaced it with standard. This adds Standard, sets up support for
it in GitHub Actions and runs it against the code base.

We ignore `Lint/ConstantDefinitionInBlock` in a few specs, as this is
related to the Pundit integration. It'd be good to come back to this in
future and adjust the specs to avoid defining/overriding classes in
specs, but honestly it was too difficult to make it work in a reasonable
time.

Finally, we need to use `main` of the standard Action as, currently,
that's the only one which is working (`0.0.5` fails silently).

https://github.com/standardrb/standard
thoughtbot/guides#606
nickcharlton added a commit to thoughtbot/administrate that referenced this pull request Feb 5, 2024
Some time ago, we at thoughtbot stopped using our custom Rubocop rules
and replaced it with standard. This adds Standard, sets up support for
it in GitHub Actions and runs it against the code base.

We ignore `Lint/ConstantDefinitionInBlock` in a few specs, as this is
related to the Pundit integration. It'd be good to come back to this in
future and adjust the specs to avoid defining/overriding classes in
specs, but honestly it was too difficult to make it work in a reasonable
time.

Finally, we need to use `main` of the standard Action as, currently,
that's the only one which is working (`0.0.5` fails silently).

https://github.com/standardrb/standard
thoughtbot/guides#606
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.

Replace Ruby style guide with standard
5 participants