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

Add base error class to new gems. #6267

Merged
merged 2 commits into from Jan 23, 2018

Conversation

christhekeele
Copy link
Contributor

@christhekeele christhekeele commented Jan 20, 2018

Closes #6260.

Room for discussion:

  • Which error class to use (StandardError makes sense to me)
  • What formatting to use (the three-lines-with-comment seemed nicest to me)
  • Whether or not using the flag to provide a different error base class is useful, and if it should validate the user's choice or not (I threw it in because it seemed harmless; is it? a boolean flag would work fine too)

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

Libraries don't always follow best practice from discussion in linked issue.

What was your diagnosis of the problem?

Bundler could encourage best practice by adding it to the gem scaffold.

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

I added a base error class to the templates, and provided a flag to change/disable this behaviour.

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

Like any best-practice-by-default, this could ruin someones workflow/go against someone's preferences so I made it as configurable as possible.

@ghost
Copy link

ghost commented Jan 20, 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

@colby-swandale
Copy link
Member

colby-swandale commented Jan 20, 2018

Thanks for opening a PR and helping make Bundler better. I apologize for saying this after you've done all this work but i imagined this PR to just be the one line from #6267. Adding in a CLI option and adding in validation just doesn't seem necessary here. It's a simple change for the user if they want to remove or use a different error as the base class.

@christhekeele
Copy link
Contributor Author

Hey, no hassle—done!

@olleolleolle
Copy link
Member

I think this looks good to go!

@colby-swandale
Copy link
Member

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 7482550 has been approved by colby-swandale

@bundlerbot
Copy link
Collaborator

⌛ Testing commit 7482550 with merge 3aa29ce...

bundlerbot added a commit that referenced this pull request Jan 23, 2018
…ndale

Add base error class to new gems.

Closes #6260.

Room for discussion:

- Which error class to use (`StandardError` makes sense to me)
- What formatting to use (the three-lines-with-comment seemed nicest to me)
- Whether or not using the flag to provide a different error base class is useful, and if it should validate the user's choice or not (I threw it in because it seemed harmless; is it? a boolean flag would work fine too)

---

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

Libraries don't always follow best practice from discussion in linked issue.

### What was your diagnosis of the problem?

Bundler could encourage best practice by adding it to the gem scaffold.

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

I added a base error class to the templates, and provided a flag to change/disable this behaviour.

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

Like any best-practice-by-default, this could ruin someones workflow/go against someone's preferences so I made it as configurable as possible.
@colby-swandale
Copy link
Member

Thanks @christhekeele for the PR!

@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: colby-swandale
Pushing 3aa29ce to master...

@bundlerbot bundlerbot merged commit 7482550 into rubygems:master Jan 23, 2018
@colby-swandale colby-swandale added this to the 1.16.2 milestone Feb 4, 2018
deivid-rodriguez added a commit to deivid-rodriguez/bundler that referenced this pull request Feb 22, 2018
@colby-swandale colby-swandale modified the milestones: 1.16.2, 1.17.0 Apr 4, 2018
colby-swandale pushed a commit that referenced this pull request Sep 20, 2018
…ndale

Add base error class to new gems.

Closes #6260.

Room for discussion:

- Which error class to use (`StandardError` makes sense to me)
- What formatting to use (the three-lines-with-comment seemed nicest to me)
- Whether or not using the flag to provide a different error base class is useful, and if it should validate the user's choice or not (I threw it in because it seemed harmless; is it? a boolean flag would work fine too)

---

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

Libraries don't always follow best practice from discussion in linked issue.

### What was your diagnosis of the problem?

Bundler could encourage best practice by adding it to the gem scaffold.

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

I added a base error class to the templates, and provided a flag to change/disable this behaviour.

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

Like any best-practice-by-default, this could ruin someones workflow/go against someone's preferences so I made it as configurable as possible.

(cherry picked from commit 3aa29ce)
colby-swandale pushed a commit that referenced this pull request Oct 5, 2018
…ndale

Add base error class to new gems.

Closes #6260.

Room for discussion:

- Which error class to use (`StandardError` makes sense to me)
- What formatting to use (the three-lines-with-comment seemed nicest to me)
- Whether or not using the flag to provide a different error base class is useful, and if it should validate the user's choice or not (I threw it in because it seemed harmless; is it? a boolean flag would work fine too)

---

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

Libraries don't always follow best practice from discussion in linked issue.

### What was your diagnosis of the problem?

Bundler could encourage best practice by adding it to the gem scaffold.

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

I added a base error class to the templates, and provided a flag to change/disable this behaviour.

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

Like any best-practice-by-default, this could ruin someones workflow/go against someone's preferences so I made it as configurable as possible.

(cherry picked from commit 3aa29ce)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider creating error base class in bundler gem
5 participants