Document and critically review `ShellError` variants - Ep. 1 by sholderbach · Pull Request #8229 · nushell/nushell

sholderbach · 2023-02-26T18:37:57Z

Description

The ShellError enum 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.

Feel free to add review comments if you spot inconsistent use of ShellError variants.

Name fields on ShellError::OperatorOverflow
Name fields on ShellError::PipelineMismatch
Add doc to ShellError::OnlySupportsThisInputType
Name ShellError::OnlySupportsThisInputType
Name field on ShellError::PipelineEmpty
Comment about issues with TypeMismatch*
Fix a few exp_input_types
Name fields on ShellError::InvalidRange

User-Facing Changes

(None now, end goal more explicit and consistent error messages)

Tests + Formatting

(No additional tests needed so far)

Instead of using tuple struct variants go for named struct variants to avoid confusion when creating `ShellError`s. The goal is to better document the errors and make them both more useful to the implementers as well as the users. A future goal of this effort is to enable serialization of errors to nushell records for finer-grained error handling.

The two variants are redundant. Naming the fields on `TypeMismatch` would be effectively the same as `TypeMismatchGenericMessage` We should make `TypeMismatch` more helpful anyways. (e.g. including both types and make sure spans are useful) When doing the joining it is worth checking for places where it is not a type error but something else.

codecov · 2023-02-26T19:04:38Z

Codecov Report

Merging #8229 (fb377ae) into main (c602b5a) will decrease coverage by 0.81%.
The diff coverage is 4.26%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8229      +/-   ##
==========================================
- Coverage   68.53%   67.73%   -0.81%     
==========================================
  Files         617      621       +4     
  Lines       99352    99287      -65     
==========================================
- Hits        68093    67252     -841     
- Misses      31259    32035     +776

Impacted Files	Coverage Δ
crates/nu-command/src/bits/and.rs	`87.01% <0.00%> (+1.11%)`	⬆️
crates/nu-command/src/bits/not.rs	`78.51% <0.00%> (ø)`
crates/nu-command/src/bits/or.rs	`87.01% <0.00%> (+1.11%)`	⬆️
crates/nu-command/src/bits/rotate_left.rs	`72.44% <0.00%> (+0.56%)`	⬆️
crates/nu-command/src/bits/rotate_right.rs	`74.04% <0.00%> (+0.56%)`	⬆️
crates/nu-command/src/bits/shift_left.rs	`70.94% <0.00%> (+0.47%)`	⬆️
crates/nu-command/src/bits/shift_right.rs	`68.11% <0.00%> (+0.49%)`	⬆️
crates/nu-command/src/bits/xor.rs	`87.01% <0.00%> (+1.11%)`	⬆️
crates/nu-command/src/bytes/add.rs	`92.70% <0.00%> (+0.67%)`	⬆️
crates/nu-command/src/bytes/at.rs	`70.94% <0.00%> (+0.30%)`	⬆️
... and 135 more

sholderbach · 2023-02-27T22:16:28Z

crates/nu-command/src/bits/and.rs

        Value::Error { .. } => value,
        other => Value::Error {
-            error: ShellError::OnlySupportsThisInputType(
-                "integer".into(),
-                other.get_type().to_string(),
-                head,
-                // This line requires the Value::Error match above.
-                other.expect_span(),
-            ),
+            error: ShellError::OnlySupportsThisInputType {
+                exp_input_type: "integer".into(),
+                wrong_type: other.get_type().to_string(),
+                dst_span: head,
+                src_span: other.expect_span(),
+            },


Thanks rust-analyzer for eating this comment in so many places... but the pervasive use of .expect_span() is a mistake in my view

I think I have something figured out to factor out that gotcha after I am done with the error message stuff. So I would save my time reverting the removal of the comments.

sholderbach · 2023-02-27T22:29:24Z

To save me from rebasing all the time I prefer to split up this effort into several PRs.

rgwood

Many, many thanks for this. It's so annoying to look at a ShellError variant and have no idea what the different fields mean. LGTM.

Continuation of #8229 # Description The `ShellError` enum 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. **Everyone:** Feel free to add review comments if you spot inconsistent use of `ShellError` variants. - Name fields of `SE::IncorrectValue` - Merge and name fields on `SE::TypeMismatch` - Name fields on `SE::UnsupportedOperator` - Name fields on `AssignmentRequires*` and fix doc - Name fields on `SE::UnknownOperator` - Name fields on `SE::MissingParameter` - Name fields on `SE::DelimiterError` - Name fields on `SE::IncompatibleParametersSingle` # User-Facing Changes (None now, end goal more explicit and consistent error messages) # Tests + Formatting (No additional tests needed so far)

Continuation of #8229 and #8326 # Description The `ShellError` enum 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 `ShellError` variants. # User-Facing Changes (None now, end goal more explicit and consistent error messages) # Tests + Formatting (No additional tests needed so far) # Commits (so far) - Remove `ShellError::FeatureNotEnabled` - Name fields on `SE::ExternalNotSupported` - Name field on `SE::InvalidProbability` - Name fields on `SE::NushellFailed` variants - Remove unused `SE::NushellFailedSpannedHelp` - Name field on `SE::VariableNotFoundAtRuntime` - Name fields on `SE::EnvVarNotFoundAtRuntime` - Name fields on `SE::ModuleNotFoundAtRuntime` - Remove usused `ModuleOrOverlayNotFoundAtRuntime` - Name fields on `SE::OverlayNotFoundAtRuntime` - Name field on `SE::NotFound`

sholderbach added 6 commits February 26, 2023 19:18

Name fields on ShellError::PipelineMismatch

56ee300

Add doc to ShellError::OnlySupportsThisInputType

f7321cb

Name ShellError::OnlySupportsThisInputType

51ddf4f

Name field on ShellError::PipelineEmpty

119732c

sholderbach added 2 commits February 27, 2023 22:30

Fix a few exp_input_types

ad79265

Name fields on ShellError::InvalidRange

fb377ae

sholderbach commented Feb 27, 2023

View reviewed changes

sholderbach changed the title ~~Document and critically review ShellError variants~~ Document and critically review ShellError variants - Ep. 1 Feb 27, 2023

sholderbach marked this pull request as ready for review February 27, 2023 22:29

rgwood approved these changes Mar 1, 2023

View reviewed changes

sholderbach merged commit 438062d into nushell:main Mar 1, 2023

sholderbach deleted the errare-humanum-est branch March 2, 2023 20:41

This was referenced Mar 5, 2023

fix: fixed typo and improved Value TypeMismatch exceptions #8324

Merged

Document and critically review ShellError variants - Ep. 2 #8326

Merged

sholderbach mentioned this pull request Mar 6, 2023

Document and critically review ShellError variants - Ep. 3 #8340

Merged

sholderbach mentioned this pull request Mar 11, 2023

Box ShellError in Value::Error #8375

Merged

sholderbach mentioned this pull request Oct 12, 2023

Make ShellError more readable by consistently using named fields #10700

Closed

46 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document and critically review `ShellError` variants - Ep. 1#8229

Document and critically review `ShellError` variants - Ep. 1#8229
sholderbach merged 8 commits intonushell:mainfrom
sholderbach:errare-humanum-est

sholderbach commented Feb 26, 2023 •

edited

Loading

Uh oh!

codecov bot commented Feb 26, 2023 •

edited

Loading

Uh oh!

sholderbach Feb 27, 2023

Uh oh!

sholderbach Mar 1, 2023

Uh oh!

sholderbach commented Feb 27, 2023

Uh oh!

rgwood left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sholderbach commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-Facing Changes

Tests + Formatting

Uh oh!

codecov bot commented Feb 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sholderbach Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

sholderbach Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

sholderbach commented Feb 27, 2023

Uh oh!

rgwood left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sholderbach commented Feb 26, 2023 •

edited

Loading

codecov bot commented Feb 26, 2023 •

edited

Loading