Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

Fix some rescue calls that do not specifiy error type.#6310

Merged
bundlerbot merged 2 commits intorubygems:masterfrom
utilum:rescue_unspecified_exception
Mar 17, 2018
Merged

Fix some rescue calls that do not specifiy error type.#6310
bundlerbot merged 2 commits intorubygems:masterfrom
utilum:rescue_unspecified_exception

Conversation

@utilum
Copy link
Copy Markdown
Contributor

@utilum utilum commented Feb 26, 2018

What was the end-user problem that led to this PR?

The problem I noticed was in style, several instances of rescue clauses that do not specify an exception type. This is noted in in the Ruby Style Guide as Avoid rescuing the Exception class.

What was your diagnosis of the problem?

My diagnosis was that at least some of those are leftover style. They are noted in .rubocop_todo.yml. To make sure, I asked on Slack.

What is your fix for the problem, implemented in this PR?

My fix is to make as many of them more specific, specifying at least StandardError, and trying to be more specific.

Why did you choose this fix out of the possible options?

No other ways of addressing this came to mind. I'd be happy to consider.

@ghost
Copy link
Copy Markdown

ghost commented Feb 26, 2018

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@utilum utilum force-pushed the rescue_unspecified_exception branch from c9a5b4f to 907689d Compare February 26, 2018 17:10
@colby-swandale
Copy link
Copy Markdown
Member

colby-swandale commented Feb 26, 2018

Hi, can you please fill out the PR template.

Copy link
Copy Markdown
Contributor

@segiddins segiddins left a comment

Choose a reason for hiding this comment

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

How did you decide between Runtime and Standard error?


Bundler.ui.warn "The latest bundler is #{latest}, but you are currently running #{current}.\n#{suggestion}"
rescue
rescue RuntimeArgument
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is “RuntimeArgument”?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Naught but a tyop. Fixed.

@utilum utilum force-pushed the rescue_unspecified_exception branch from 907689d to 2b8a80c Compare February 28, 2018 08:50
@utilum
Copy link
Copy Markdown
Contributor Author

utilum commented Feb 28, 2018

How did you decide between Runtime and Standard error?

I preferred RuntimeError as it is more specific than StandardError, won't rescue a SystemCallError for example.
When using RuntimeError caused a failure of the tests and I was not certain which narrower Exception may need rescuing, I resorted to StandardError. Perhaps those cases are redundant? Would you have them removed?

@indirect
Copy link
Copy Markdown

indirect commented Mar 4, 2018

This looks good to me once the tests are green. 👍

@colby-swandale
Copy link
Copy Markdown
Member

ping @utilum

@utilum
Copy link
Copy Markdown
Contributor Author

utilum commented Mar 9, 2018

Sorry for the delay.
I don't have Ruby 1.8.7 locally, and that's where the test failures occur. Just looking at the test log I wonder if the fix to this would not actually be in updating the tests... Anyone?

@utilum
Copy link
Copy Markdown
Contributor Author

utilum commented Mar 16, 2018

Anyone?

@segiddins segiddins force-pushed the rescue_unspecified_exception branch from 2b8a80c to c18c48d Compare March 17, 2018 20:28
@segiddins
Copy link
Copy Markdown
Contributor

@bundlerbot r+

@bundlerbot
Copy link
Copy Markdown
Collaborator

📌 Commit c18c48d has been approved by segiddins

@bundlerbot
Copy link
Copy Markdown
Collaborator

⌛ Testing commit c18c48d with merge ff6b871...

bundlerbot added a commit that referenced this pull request Mar 17, 2018
Fix some rescue calls that do not specifiy error type.

### What was the end-user problem that led to this PR?

The problem I noticed was in style, several instances of `rescue` clauses that do not specify an exception type. This is noted in in the Ruby Style Guide as [Avoid rescuing the Exception class](https://github.com/bbatsov/ruby-style-guide#no-blind-rescues).

### What was your diagnosis of the problem?

My diagnosis was that at least some of those are leftover style. They are noted in `.rubocop_todo.yml`. To make sure, I asked on Slack.

### What is your fix for the problem, implemented in this PR?

My fix is to make as many of them more specific, specifying at least `StandardError`, and trying to be more specific.

### Why did you choose this fix out of the possible options?

No other ways of addressing this came to mind. I'd be happy to consider.
@bundlerbot
Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-travis
Approved by: segiddins
Pushing ff6b871 to master...

@bundlerbot bundlerbot merged commit c18c48d into rubygems:master Mar 17, 2018
@colby-swandale colby-swandale added this to the 1.17.1 milestone Sep 24, 2018
colby-swandale pushed a commit that referenced this pull request Oct 5, 2018
Fix some rescue calls that do not specifiy error type.

The problem I noticed was in style, several instances of `rescue` clauses that do not specify an exception type. This is noted in in the Ruby Style Guide as [Avoid rescuing the Exception class](https://github.com/bbatsov/ruby-style-guide#no-blind-rescues).

My diagnosis was that at least some of those are leftover style. They are noted in `.rubocop_todo.yml`. To make sure, I asked on Slack.

My fix is to make as many of them more specific, specifying at least `StandardError`, and trying to be more specific.

No other ways of addressing this came to mind. I'd be happy to consider.

(cherry picked from commit ff6b871)
colby-swandale pushed a commit that referenced this pull request Oct 5, 2018
Fix some rescue calls that do not specifiy error type.

The problem I noticed was in style, several instances of `rescue` clauses that do not specify an exception type. This is noted in in the Ruby Style Guide as [Avoid rescuing the Exception class](https://github.com/bbatsov/ruby-style-guide#no-blind-rescues).

My diagnosis was that at least some of those are leftover style. They are noted in `.rubocop_todo.yml`. To make sure, I asked on Slack.

My fix is to make as many of them more specific, specifying at least `StandardError`, and trying to be more specific.

No other ways of addressing this came to mind. I'd be happy to consider.

(cherry picked from commit ff6b871)
colby-swandale pushed a commit that referenced this pull request Oct 7, 2018
* 1-16-stable:
  Version 1.16.6 with changelog
  fix uninitialized @use_gvp instance var warning
  no longer test Ruby 1.9.3 against rubygems master
  Merge #6708
  Auto merge of #6697 - walf443:added_changelog_section, r=hsbt
  Merge #6687
  Merge #6686
  Auto merge of #6670 - bundler:colby/invite-stephanie-morillo, r=segiddins
  Auto merge of #6627 - agrim123:agr-fix-add-groups, r=deivid-rodriguez
  Auto merge of #6612 - hdf1986:readme-bundle-add, r=segiddins
  Auto merge of #6495 - bundler:segiddins/6491-extra-gem-platform-in-lockfile, r=segiddins
  Auto merge of #6493 - agrim123:agr-update-bundle-update-docs, r=colby-swandale
  Auto merge of #6310 - utilum:rescue_unspecified_exception, r=segiddins
  Auto merge of #6184 - arbonap:pa-check-in-gemfile-docs, r=indirect
  fix typo
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.

5 participants