Document and critically review ShellError variants - Ep. 3#8340
Merged
sholderbach merged 29 commits intonushell:mainfrom Mar 6, 2023
Merged
Document and critically review ShellError variants - Ep. 3#8340sholderbach merged 29 commits intonushell:mainfrom
ShellError variants - Ep. 3#8340sholderbach merged 29 commits intonushell:mainfrom
Conversation
Not used in the whole repository
Consider dropping completely as it is a single use variant. Updated the help label a bit.
Added a help message that informs about the fatal nature of the issues observed. Q: @kubouch I wasn't sure if some usages e.g. in `use` are not strictly a moment where nushell failed over.
Might be worth unifying or updating the `NotFound` variants
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8340 +/- ##
==========================================
- Coverage 68.56% 68.13% -0.44%
==========================================
Files 620 620
Lines 99275 99429 +154
==========================================
- Hits 68070 67743 -327
- Misses 31205 31686 +481
|
Guaranteed source of confusion with the order of the types now fixed. Have only inspected the dataframe related changes I had to fix manually due to my currently borked rust-analyzer
Maybe make a more general type error if applicable.
46 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuation of #8229 and #8326
Description
The
ShellErrorenum at the moment is kind of messy.Many variants are basic tuple structs where you always have to reference the implementation with its macro invocation to know which field serves which purpose.
Furthermore we have both variants that are kind of redundant or either overly broad to be useful for the user to match on or overly specific with few uses.
So I set out to start fixing the lacking documentation and naming to make it feasible to critically review the individual usages and fix those.
Furthermore we can decide to join or split up variants that don't seem to be fit for purpose.
Call to action
Everyone: Feel free to add review comments if you spot inconsistent use of
ShellErrorvariants.User-Facing Changes
(None now, end goal more explicit and consistent error messages)
Tests + Formatting
(No additional tests needed so far)
Commits (so far)
ShellError::FeatureNotEnabledSE::ExternalNotSupportedSE::InvalidProbabilitySE::NushellFailedvariantsSE::NushellFailedSpannedHelpSE::VariableNotFoundAtRuntimeSE::EnvVarNotFoundAtRuntimeSE::ModuleNotFoundAtRuntimeModuleOrOverlayNotFoundAtRuntimeSE::OverlayNotFoundAtRuntimeSE::NotFound