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

Allow inherit_from to accept a glob #11261

Merged

Conversation

alexevanczuk
Copy link
Contributor

@alexevanczuk alexevanczuk commented Dec 9, 2022

This PR allows inherit_from to accept a glob.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

Benchmarking

I ran benchmarks on rails to determine if this change causes a performance regression.

10x bundle exec rubocop --show-cop=Style/EmptyMethod

This reflects the basic startup time of Rubocop to process configurations and resolve inheritance.

Before

real    0m1.325s
real    0m1.260s
real    0m1.255s
real    0m1.293s
real    0m1.241s
real    0m1.240s
real    0m1.220s
real    0m1.147s
real    0m1.168s
real    0m1.211s

After

real    0m1.164s
real    0m1.213s
real    0m1.203s
real    0m1.200s
real    0m1.126s
real    0m1.198s
real    0m1.194s
real    0m1.191s
real    0m1.199s
real    0m1.200s

Conclusion

No change.

10x bundle exec rubocop

This reflects overall rubocop execution time.

Before

real    0m3.245s
real    0m3.169s
real    0m2.994s
real    0m2.898s
real    0m2.820s
real    0m2.848s
real    0m2.926s
real    0m3.006s
real    0m3.215s
real    0m3.227s

After

real    0m3.258s
real    0m2.964s
real    0m2.986s
real    0m2.966s
real    0m2.835s
real    0m2.957s
real    0m3.163s
real    0m2.839s
real    0m2.970s
real    0m2.911s

Conclusion

No change.

@alexevanczuk alexevanczuk force-pushed the ae-allow-inherit-to-to-accept-glob branch from 4e65f7c to 9443318 Compare December 9, 2022 15:56
@bbatsov
Copy link
Collaborator

bbatsov commented Dec 19, 2022

I'm wondering if in some cases the order in which the extra files get required won't be an issue. Any thoughts on this?

@@ -114,6 +114,13 @@ inherit_from:
- ../conf/.rubocop.yml
----

`inherit_from` also accepts a glob, for example:
[source,yaml]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add an extra blank here.

@@ -114,6 +114,13 @@ inherit_from:
- ../conf/.rubocop.yml
----

`inherit_from` also accepts a glob, for example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might also want to say a few words about potential use-cases here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some more text about using this to manage per-component .rubocop_todo.yml files. I'm not actually sure about other use cases since that use-case was the main motivator here 🤔 Let me know what ya think!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me.

@alexevanczuk alexevanczuk force-pushed the ae-allow-inherit-to-to-accept-glob branch from bdc4f58 to e57f3e5 Compare December 19, 2022 13:53
Copy link
Collaborator

@jonas054 jonas054 left a comment

Choose a reason for hiding this comment

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

Nice feature! Looks good. 👍

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 20, 2022

@alexevanczuk The CI's failing because a few lines are now too long.

@alexevanczuk
Copy link
Contributor Author

@alexevanczuk The CI's failing because a few lines are now too long.

@bbatsov Thanks! Fixed. I'm having trouble seeing the results from CI. When I click the ❌ next to CircleCI Pipeline I just see:

Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.
Please sign out of CircleCI and log back in with your VCS before triggering a new pipeline.

Let me know if there's something I should be doing differently!

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 21, 2022

Here's what we got from the CircleCI support team.

Contributors who get this error need to re-authenticate with CircleCI because their GitHub OAuth token has expired.
https://support.circleci.com/hc/en-us/articles/360051228052-How-to-Perform-a-Re-Authentication

Alex Evanczuk added 2 commits December 21, 2022 08:34
This reverts commit e5bafbe.
@alexevanczuk
Copy link
Contributor Author

Here's what we got from the CircleCI support team.

Contributors who get this error need to re-authenticate with CircleCI because their GitHub OAuth token has expired.
https://support.circleci.com/hc/en-us/articles/360051228052-How-to-Perform-a-Re-Authentication

Thanks @bbatsov . I reauthorized on circle CI and made some good progress. Now I'm just seeing...

  1. A failure for cc-upload-coverage:
Error: response from https://api.codeclimate.com/v1/test_reports.
HTTP 401: You are not authorized for this action
Usage:
  cc-test-reporter upload-coverage [flags]
  1. Some workflows that need to be approved it look like

@bbatsov bbatsov merged commit 15dee27 into rubocop:master Dec 28, 2022
@bbatsov
Copy link
Collaborator

bbatsov commented Dec 28, 2022

Seems we're in business now. Thanks!

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.

None yet

3 participants