Skip to content

Disable pending cops when running rubocop#9136

Merged
jekyllbot merged 10 commits intojekyll:masterfrom
yboulkaid:rubocop
Sep 29, 2022
Merged

Disable pending cops when running rubocop#9136
jekyllbot merged 10 commits intojekyll:masterfrom
yboulkaid:rubocop

Conversation

@yboulkaid
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants