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 error when trying to display error message #11742

Merged

Conversation

composerinteralia
Copy link
Contributor

@composerinteralia composerinteralia commented Mar 29, 2023

When RuboCop errors, it prints a message that includes the issue tracker URL. It pulls this URL from the gem spec metadata.

Relying on the gem spec can cause problems in bundler standalone mode, where everything is set up by the standalone file and Gem.loaded_specs is not populated with the bundled gems.

$ bundle init
$ bundle add rubocop --skip-install
$ bundle install --standalone
$ ruby -r ./bundle/bundler/setup.rb -e 'require "rubocop"; Gem.loaded_specs["rubocop"].metadata['bug_tracker_uri']'
undefined method `metadata' for nil:NilClass (NoMethodError)

This means that if RuboCop errors, instead of a helpful error message we get a cryptic error that masks the underlying problem. We've seen this a number of times at GitHub (although mostly when developing our own internal cops and not from bugs in RuboCop itself).

This commit makes the error reporting more resilient by skipping the issue tracker URL when the rubocop spec is unavailable. Leaving out the URL in this case seems OK to me, since it's basically falling back to the error message before ef74a18. Hard-coding the URL is another option, although there's some risk of it falling out of sync some day.

I didn't find any existing test coverage for this code (perhaps because it only gets run if something else is broken), so I haven't added any additional coverage here.


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. (See note above. Open to suggestions!)
  • 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.

When RuboCop errors, it prints a message that includes the issue tracker
URL. It pulls this URL from the gem spec metadata.

Relying on the gem spec can cause problems in bundler standalone mode,
where everything is set up by the standalone file and `Gem.loaded_specs`
is not populated with the bundled gems.

```
$ bundle init
$ bundle add rubocop --skip-install
$ bundle install --standalone
$ ruby -r ./bundle/bundler/setup.rb -e 'require "rubocop"; Gem.loaded_specs["bundler"].metadata['bug_tracker_uri']'
undefined method `metadata' for nil:NilClass (NoMethodError)
```

This means that if RuboCop errors, instead of a helpful error message we
get a cryptic error that masks the underlying problem. We've seen this a
number of times at GitHub (although mostly when developing our own
internal cops and not from bugs in RuboCop itself).

This commit makes the error reporting more resilient by skipping the
issue tracker URL when the rubocop spec is unavailable. Leaving out the
URL in this case seems OK to me, since it's basically falling back to
the error message before rubocop@ef74a18.
Hard-coding the URL is another option, although there's some risk of it
falling out of sync some day.

I didn't find any existing test coverage for this code (perhaps because
it only gets run if something else is broken), so I haven't added any
additional coverage here.
@composerinteralia composerinteralia force-pushed the bundler-standalone-error-reporting branch from fa6574c to d6bc76a Compare March 29, 2023 00:18
@koic koic merged commit 4d52c6a into rubocop:master Mar 29, 2023
11 checks passed
@koic
Copy link
Member

koic commented Mar 29, 2023

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

2 participants