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

Consolidate errors to streamline nice error quest issue. #249

Merged
merged 23 commits into from Feb 26, 2019

Conversation

@charlespierce
Copy link
Contributor

@charlespierce charlespierce commented Jan 30, 2019

Info

  • To make it easier to tackle improving the error messages across the app, we want to streamline the process of adding a new error message.
  • One approach to that is to Consolidate all of the errors into a single ErrorDetails enum, so that new errors can simply be added to that enum, as opposed to creating entirely new structs throughout the app.

Changes

  • Add a new module error that exposes an InternalError enum which implements NotionFail
  • Move error objects in fs into the enum
  • Move error objects in inventory into the enum
  • Move error objects in manifest into the enum
  • Move error objects in path into the enum
  • Move error objects in project into the enum
  • Move error objects in session into the enum
  • Move error objects in shell into the enum
  • Move error objects in shim into the enum
  • Move error objects in tool into the enum
  • Move error objects in version into the enum
  • Move error objects in root src into the enum
  • Restore comments about the intent of some errors

Notes

  • I also removed the shim command, it wasn't compiling after other changes and will shortly be replaced by the Package Installs.
  • The approach to adding a new Error message is now:
    1. Add a new entry to the ErrorDetails enum, including information you need for the error message if necessary.
    2. Update the match block in the impl Display for ErrorDetails block to include the error message for the new error.
    3. Update the match block in the impl NotionFail for ErrorDetails block to return the correct ExitCode for the new error.
    4. Replace any .unknown() calls that should wrap an failure in the new error with a .with_context(|error| ErrorDetails::NewError) call, setting any contextual values needed.
@charlespierce charlespierce changed the title [WIP] Consolidate errors to streamline nice error quest issue. Consolidate errors to streamline nice error quest issue. Feb 1, 2019
@charlespierce charlespierce requested review from dherman and mikrostew Feb 1, 2019
Copy link
Contributor

@chriskrycho chriskrycho left a comment

I have a handful of notes, a couple suggestions, and one important change. This is really fantastic.

crates/notion-core/src/distro/mod.rs Outdated Show resolved Hide resolved
crates/notion-core/src/error.rs Show resolved Hide resolved
crates/notion-core/src/fs.rs Show resolved Hide resolved
crates/notion-core/src/fs.rs Outdated Show resolved Hide resolved
crates/notion-core/src/shell/mod.rs Outdated Show resolved Hide resolved
crates/notion-core/src/shell/mod.rs Show resolved Hide resolved
crates/notion-core/src/shell/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@chriskrycho chriskrycho left a comment

💯

@chriseppstein
Copy link

@chriseppstein chriseppstein commented Feb 22, 2019

I want these nice errors. I am tired of the mean errors.

Copy link
Collaborator

@dherman dherman left a comment

This looks great. A few suggestions on naming and streamlining a helper.

crates/notion-core/src/distro/mod.rs Outdated Show resolved Hide resolved
crates/notion-core/src/shim.rs Outdated Show resolved Hide resolved
crates/notion-core/src/tool/mod.rs Outdated Show resolved Hide resolved
crates/notion-core/src/version/mod.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
@charlespierce
Copy link
Contributor Author

@charlespierce charlespierce commented Feb 25, 2019

Updated to rename all the top-level error context functions 👍

Copy link
Collaborator

@dherman dherman left a comment

Looks great!

@dherman dherman merged commit 64bc1cf into volta-cli:master Feb 26, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@charlespierce charlespierce deleted the charlespierce:nice_errors branch Feb 26, 2019
@charlespierce charlespierce mentioned this pull request Mar 8, 2019
14 of 14 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants