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

Fix warning from upstream rubocop #57

Closed
wants to merge 5 commits into from

Conversation

ipmsteven
Copy link

@ipmsteven ipmsteven commented Apr 18, 2020

The .rubocop.yml for rubocop-github itself is:
`inherit_from: ./config/default.yml`, which
`inherit_from: default_deprecated.yml`, which
`inherit_from: _default_shared.yml`, which is using some Performance cop
there.
Copy link

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing the cleanup here! Outdated namespaces and duplication... always good to fix these up (and get rid of warnings!).

I have a couple of questions/suggestions.

@@ -1,5 +1,8 @@
inherit_from: ./config/default.yml

require:
- rubocop-performance

Choose a reason for hiding this comment

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

Are some of the renamed checks in Rubocop Performance, or is this a separate change?

Copy link
Author

Choose a reason for hiding this comment

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

hmm, we are using some Performance cops in the .rubocop.yml of rubocop-github itself, and I got errors when rubocop-performance is not required

- 2.4.6
- 2.5.5
- 2.6.3
- 2.7.1

Choose a reason for hiding this comment

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

For future debugging purposes, can we put CI changes in a separate PR?

Copy link
Author

Choose a reason for hiding this comment

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

sure, let me bring it in separate PR then

Copy link
Author

@ipmsteven ipmsteven Apr 20, 2020

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

It looks like we'll need to do this PR first, then the CI PR since CI is breaking on the warnings :)

Copy link
Author

Choose a reason for hiding this comment

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

yes, but I got CI test failure on 2.3 for this PR :(

Choose a reason for hiding this comment

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

It looks like on 2.3 it failed with Layout/IndentationStyle:

https://travis-ci.org/github/github/rubocop-github/jobs/676500683

Which is odd, since you fixed that. I didn't chase it further (maybe there's another place where it's being used?)

@kytrinyx kytrinyx requested a review from a team April 20, 2020 18:28
@ipmsteven
Copy link
Author

close this issue and the changes will be handled in following PRs:

#60
#61
#62

@ipmsteven ipmsteven closed this Apr 21, 2020
@ipmsteven ipmsteven deleted the fix-warning branch April 21, 2020 17:25
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

2 participants