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

Disable pending cops when running rubocop #9136

Merged
merged 10 commits into from Sep 29, 2022
Merged

Conversation

yboulkaid
Copy link
Contributor

@yboulkaid yboulkaid commented Sep 28, 2022

Summary

In #9125 we bumped the rubocop version to 1.36, which introduces new
cops. Unconfigured cops output a warning when running script/fmt,
which was noisy.

This PR makes rubocop run with the --disable-pending-cops option
to make the script output cleaner.

@yboulkaid yboulkaid marked this pull request as ready for review September 28, 2022 18:38
@ashmaroli
Copy link
Member

I am on the fence regarding this.

I refrained from enabling certain cops (ones concerned with patterns and numbered parameters) in the aforementioned pull request because they were related to Ruby 3 syntax which Jekyll has not yet adopted.
The remaining ones revolving minitest and rspec seemed noisy.

@yboulkaid
Copy link
Contributor Author

yboulkaid commented Sep 29, 2022

I refrained from enabling certain cops (ones concerned with patterns and numbered parameters) in the aforementioned pull request because they were related to Ruby 3 syntax which Jekyll has not yet adopted.

Aha I see. Is there a problem enabling them even if Jekyll's syntax is still following ruby 2.x? They will not be triggered until someone tries the new syntax, right?

The remaining ones revolving minitest and rspec seemed noisy.

Do you mean noisy as entries in rubocop.yml, the cop recommendations themselves?

I am not very opinionated when it comes to which cops should be enabled or not (especially since I'm new to the codebase). The main point of this PR is to have a warning-free script/fmt. Another way to achieve this can be to run rubocop with --disable-pending-cops which disables all the new cops, do you think it's a more fitting solution?

@ashmaroli
Copy link
Member

Is there a problem enabling them even if Jekyll's syntax is still following ruby 2.x?

Personally, I feel enabling unnecessary cops means redundancy.

Another way to achieve this can be to run rubocop with --disable-pending-cops which disables all the new cops

I like this alternative. Especially since that is your primary goal with this PR.
Let us go with adding this flag to the shell script and updating the PR title and description to reflect the intention.

@yboulkaid yboulkaid changed the title Update rubocop config with new cops Disable pending cops when running rubocop Sep 29, 2022
@yboulkaid
Copy link
Contributor Author

Personally, I feel enabling unnecessary cops means redundancy.

I buy the argument 👍

I like this alternative. Especially since that is your primary goal with this PR.
Let us go with adding this flag to the shell script and updating the PR title and description to reflect the intention.

Done :)

@ashmaroli
Copy link
Member

Thanks @yboulkaid
@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit daca7e5 into jekyll:master Sep 29, 2022
jekyllbot added a commit that referenced this pull request Sep 29, 2022
github-actions bot pushed a commit that referenced this pull request Sep 29, 2022
Youssef Boulkaid: Disable pending cops when running rubocop (#9136)

Merge pull request 9136
@jekyll jekyll locked and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants