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

Update rubocop namespaces for 0.78.x #1096

Conversation

NathanielRN
Copy link
Contributor

Description

I was using VSCode and formatting with the rubocop extension when I got a warning for the namespace of the instrumentation/.rubocop-examples.yml.

I looked at this issue comment and found that it should be Layout/LineLength in that file. Updated it in this PR to make the warning go away.

Copy link
Contributor

@arielvalentin arielvalentin left a comment

Choose a reason for hiding this comment

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

The version of rubocop for all of our instrumentations is constrained to bug fix releases of 0.73.x

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/.instrumentation_generator/templates/gemspec.tt#L36

In that version LineLength is still under metrics https://github.com/rubocop/rubocop/blob/v0.73.0/lib/rubocop/cop/metrics/line_length.rb

You would would have to upgrade to everything to v0.78.0 or newer in order for us to accept this change.

https://github.com/rubocop/rubocop/blob/v0.78.0/lib/rubocop/cop/layout/line_length.rb

@NathanielRN NathanielRN force-pushed the update-rubocop-line-length-namespace branch from 3635ad3 to 2117d40 Compare January 21, 2022 03:12
@NathanielRN NathanielRN changed the title Update rubocop line length namespace Update rubocop namespaces for 0.78.x Jan 21, 2022
@NathanielRN
Copy link
Contributor Author

Ah thank you for the explanation! I'll work on changing my rubocop version in the meantime so I can be consistent :)

I wanted to try to upgrade it! So I tried to upgrade to 0.78.x by updating the target and then updating the namespaces after I ran rubocop until there were no naming errors. (Curiously enough it did not complain about Metrics/LineLength...

However, I found that there were a lot of lint issues issues it found, most of which could be "safely" corrected. I did run the auto corrector on my local machine but I'm worried this would be pretty hard to review and touch a lot of the git history:

57 files changed, 582 insertions(+), 554 deletions(-)

Even with these changes there are still 177 "unsafe" changes, and 164 changes that can't automatically be fixed:

931 files inspected, 1257 offenses detected, 916 offenses corrected, 117 more offenses can be corrected with `rubocop -A`

Let me know how the SIG handles upgrades like these? Or if not it's fine to close this PR :)

@arielvalentin
Copy link
Contributor

😩 that seems like a lot of changes. We have not gone through the process of upgrading rubocop yet and I do not know the history of selecting 0.73 version for this project so I think we need to gain some insights from @open-telemetry/ruby-maintainers.

What concerns me is that there are other linters that were relocated/renamed and are now reporting violations of things we purposely have ignored. I am also unclear as to what happens if a cop is renamed or deprecated. Will it fail?

@dazuma
Copy link
Member

dazuma commented Jan 21, 2022

@arielvalentin So the history... old versions of rubocop (before sometime in the 0.80s or something around there) would add—and automatically enable—new checks on every minor version, which would effectively be a breaking change because it would fail for things that didn't fail before. As a result, we pinned rubocop to a minor release, with the idea that we'd periodically update it, and each time we'd just go through the exercise of updating a bunch of code in the process to conform. 0.73 must simply have been the last time we did that, 2 or 3 years ago.

Of course, at some point later, the rubocop maintainers realized this was a problem, and reworked their configs so that new checks were disabled by default. So now, if we update rubocop to something relatively recent (definitely 1.0, and I think some minor 0.x versions) we shouldn't run into this issue anymore and we can to float to the latest minor release. But that will, of course, require us to incur the one time cost of updating a lot of code to conform to newer lint checks.

I do think we need to do this, because my understanding is that older versions of rubocop have issues with Ruby 3.0 and 3.1.

@arielvalentin
Copy link
Contributor

@dazuma thank you for the detailed history there. That was helpful!

So it does indeed sound like we want to upgrade to the most recent version if possible. @NathanielRN do you have the cycles (and patience) to help us with this?

@NathanielRN
Copy link
Contributor Author

I wouldn't mind helping! 🙂 But I also wrote my first line of Ruby ever this week 😅 If the task is just to do those 177 + 164 lint errors I could go ahead and try, but like I said it would probably be a change that needs to be reviewed.

@arielvalentin
Copy link
Contributor

Learn Ruby the Hard Way ™️ 😂

@NathanielRN NathanielRN marked this pull request as draft January 28, 2022 04:06
@plantfansam
Copy link
Contributor

Hello, and thank you for your contribution!

We recently split Ruby instrumentation out into the opentelemetry-ruby-contrib repo.

This PR is related to instrumentation, so we'll need you to re-open it against opentelemetry-ruby-contrib. Sorry for the inconvenience!

To do that, you can:

  1. Create a fork of opentelemetry-ruby-contrib and copy the git url
  2. In your opentelemetry-ruby repo, run git remote add tmp-contrib <your-fork-git-url>
  3. git push tmp-contrib your-branch-name
  4. Open a new PR in contrib (feel free to just copy/paste your original PR description there)
  5. Close your open PR in this repo with a comment that links to your new PR in contrib
  6. Delete your tmp-contrib remote from opentelemetry-ruby (git remote rm tmp-contrib)
  7. git clone your opentelemetry-ruby-contrib fork, check out your branch, and make all changes in that repo from now on!

Sorry again for the inconvenience, and thank you for contributing!

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

4 participants