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

Make ShellError more readable by consistently using named fields #10700

Closed
33 of 46 tasks
sholderbach opened this issue Oct 12, 2023 · 6 comments · Fixed by #11276
Closed
33 of 46 tasks

Make ShellError more readable by consistently using named fields #10700

sholderbach opened this issue Oct 12, 2023 · 6 comments · Fixed by #11276
Labels
help wanted Extra attention is needed meta-issue An issue that tracks other issues unhelpful-error The error message you observe is not helpful to identify the problem
Milestone

Comments

@sholderbach
Copy link
Member

sholderbach commented Oct 12, 2023

Our ShellError still has a number of variants that are tuple structs (i.e. unnamed list of types to provide). Especially if there are multiple fields of the same type it is unclear at the creation side of a ShellError which field goes where. This is exacerbated by the fact that for the miette diagnostics we have sometimes format strings that make strong assumptions what should be provided for a particular field.

Thus we should convert each of them to named struct variants (i.e. Identifier { field: type, ... }). While doing so it is worth checking if we use the fields of the error correctly. Sometimes we might need to change incompatible uses of a variant to a different variant.

Variants that need named fields

  • UnsupportedInput
  • DatetimeParseError
  • NetworkFailure
  • CommandNotFound
  • AliasNotFound
    - FlagNotFound
  • FileNotFound
  • FileNotFoundCustom
  • PluginFailedToLoad
  • PluginFailedToEncode
  • PluginFailedToDecode
  • IOInterrupted
  • IOError
  • IOErrorSpanned
  • PermissionDeniedError
  • OutOfMemoryError
  • NotADirectory
  • DirectoryNotFound
  • DirectoryNotFoundCustom
  • MoveNotPossibleSingle
  • CreateNotPossible
  • ChangeAccessTimeNotPossible
  • ChangeModifiedTimeNotPossible
  • RemoveNotPossible
  • NoFileToBeRemoved
  • NoFileToBeMoved
  • NoFileToBeCopied
  • ReadingFile
  • DidYouMean
  • DidYouMeanCustom
  • NonUtf8
  • NonUtf8Custom
  • DowncastNotPossible
  • UnsupportedConfigValue
  • MissingConfigValue
  • NeedsPositiveValue
  • GenericError
  • OutsideSpannedLabeledError
  • RemovedCommand
  • UnexpectedAbbrComponent
  • EvalBlockWithInput
  • Break
  • Continue
  • Return
  • NotAConstant
  • NotAConstCommand
  • NotAConstHelp

Example PRs

@sholderbach sholderbach added help wanted Extra attention is needed meta-issue An issue that tracks other issues unhelpful-error The error message you observe is not helpful to identify the problem labels Oct 12, 2023
@fdncred
Copy link
Collaborator

fdncred commented Oct 12, 2023

It would be nice to pin this but I'm not sure which other pinned issue to remove.

Would love to have more consistency across our ShellError messages!

@kaiba42
Copy link

kaiba42 commented Oct 19, 2023

tldr; ShellError variants should have record<> types for fields instead of strings.

I'm having some trouble dealing with these ShellError variants in try { cmd } catch{|e| ...} blocks. When cmd fails, the type of $e is record<msg: string, debug: string, raw: error>, which I can see is mapped from ShellError::ExternalCommand.

The problem is that the fields within the record, the debug string is formatted as

ExternalCommand { label: "some string", help: "some error", span: Span { start: 12345, end: 67890 } }

Which isn't able to be parsed into a structured data type to my knowledge. However, it clearly is more than just a string to nushell, because running metadata ($e.debug) will actually yield a record<span: record<start: int, end: int>>, which suggests that $e.debug is not merely a string. I think $e.debug should be a record, not a string.

@sholderbach
Copy link
Member Author

@kaiba42 there is certainly some extra work necessary to make sure as a Nushell user you have access to the relevant fields.

But, yes completing this internal polishing here should make it easier to cleanly serialize the internal error to a Nushell record.

drbrain added a commit to drbrain/nushell that referenced this issue Nov 6, 2023
This is easy to do with rust-analyzer, but I didn't want to just pump
these all out without feedback.

Part of nushell#10700
sholderbach added a commit that referenced this issue Nov 7, 2023
# Description

This is easy to do with rust-analyzer, but I didn't want to just pump
these all out without feedback.

Part of #10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A

---------

Co-authored-by: Stefan Holderbach <sholderbach@users.noreply.github.com>
drbrain added a commit to drbrain/nushell that referenced this issue Nov 8, 2023
sholderbach pushed a commit that referenced this issue Nov 8, 2023
# Description

Part of #10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
drbrain added a commit to drbrain/nushell that referenced this issue Nov 18, 2023
drbrain added a commit to drbrain/nushell that referenced this issue Nov 19, 2023
drbrain added a commit to drbrain/nushell that referenced this issue Nov 19, 2023
drbrain added a commit to drbrain/nushell that referenced this issue Nov 19, 2023
sholderbach pushed a commit that referenced this issue Nov 19, 2023
sholderbach pushed a commit that referenced this issue Nov 19, 2023
drbrain added a commit to drbrain/nushell that referenced this issue Nov 21, 2023
drbrain added a commit to drbrain/nushell that referenced this issue Nov 21, 2023
drbrain added a commit to drbrain/nushell that referenced this issue Nov 21, 2023
WindSoilder pushed a commit that referenced this issue Nov 21, 2023
# Description

`ShellError::FlagNotFound` had a note that said it may be removable so
this PR removes it instead of updating it to named fields per #10700

I can't see this error being used since it was introduced with #4364. I
can't find why or where it was used before that date, though. There was
a large merge with that PR but I can't penetrate the secrets of git to
find out where its earlier history went.

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
WindSoilder pushed a commit that referenced this issue Nov 21, 2023
# Description

Part of #10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
WindSoilder pushed a commit that referenced this issue Nov 21, 2023
# Description

Part of #10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
@hustcer hustcer added this to the v0.88.0 milestone Dec 10, 2023
@amtoine
Copy link
Member

amtoine commented Dec 10, 2023

@drbrain
there are 13 unticked tasks in this issue, should it be reopened?

hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

This is easy to do with rust-analyzer, but I didn't want to just pump
these all out without feedback.

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A

---------

Co-authored-by: Stefan Holderbach <sholderbach@users.noreply.github.com>
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

`ShellError::FlagNotFound` had a note that said it may be removable so
this PR removes it instead of updating it to named fields per nushell#10700

I can't see this error being used since it was introduced with nushell#4364. I
can't find why or where it was used before that date, though. There was
a large merge with that PR but I can't penetrate the secrets of git to
find out where its earlier history went.

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Convert these ShellError variants to named fields:
* CreateNotPossible
* MoveNotPossibleSingle
* DirectoryNotFoundCustom
* DirectoryNotFound
* NotADirectory
* OutOfMemoryError
* PermissionDeniedError
* IOErrorSpanned
* IOError
* IOInterrupted

Also place the `span` field of `DirectoryNotFound` last to match other
errors.

Part of nushell#10700 (almost half done!)

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Convert errors to named fields:
* NeedsPositiveValue
* MissingConfigValue
* UnsupportedConfigValue
* DowncastNotPossible
* NonUtf8Custom
* NonUtf8
* DidYouMeanCustom
* DidYouMean
* ReadingFile
* RemoveNotPossible
* ChangedModifiedTimeNotPossible
* ChangedAccessTimeNotPossible

Part of nushell#10700
hardfau1t pushed a commit to hardfau1t/nushell that referenced this issue Dec 14, 2023
# Description

Removed variants that are no longer in use:
* `NoFile*`
* `UnexpectedAbbrComponent`

Converted:
* `OutsideSpannedLabeledError`
* `EvalBlockWithInput`
* `Break`
* `Continue`
* `Return`
* `NotAConstant`
* `NotAConstCommand`
* `NotAConstHelp`
* `InvalidGlobPattern`
* `ErrorExpandingGlob`

Fixes nushell#10700 

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

This is easy to do with rust-analyzer, but I didn't want to just pump
these all out without feedback.

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A

---------

Co-authored-by: Stefan Holderbach <sholderbach@users.noreply.github.com>
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

`ShellError::FlagNotFound` had a note that said it may be removable so
this PR removes it instead of updating it to named fields per nushell#10700

I can't see this error being used since it was introduced with nushell#4364. I
can't find why or where it was used before that date, though. There was
a large merge with that PR but I can't penetrate the secrets of git to
find out where its earlier history went.

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Part of nushell#10700

# User-Facing Changes

None

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Convert these ShellError variants to named fields:
* CreateNotPossible
* MoveNotPossibleSingle
* DirectoryNotFoundCustom
* DirectoryNotFound
* NotADirectory
* OutOfMemoryError
* PermissionDeniedError
* IOErrorSpanned
* IOError
* IOInterrupted

Also place the `span` field of `DirectoryNotFound` last to match other
errors.

Part of nushell#10700 (almost half done!)

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Convert errors to named fields:
* NeedsPositiveValue
* MissingConfigValue
* UnsupportedConfigValue
* DowncastNotPossible
* NonUtf8Custom
* NonUtf8
* DidYouMeanCustom
* DidYouMean
* ReadingFile
* RemoveNotPossible
* ChangedModifiedTimeNotPossible
* ChangedAccessTimeNotPossible

Part of nushell#10700
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this issue Feb 20, 2024
# Description

Removed variants that are no longer in use:
* `NoFile*`
* `UnexpectedAbbrComponent`

Converted:
* `OutsideSpannedLabeledError`
* `EvalBlockWithInput`
* `Break`
* `Continue`
* `Return`
* `NotAConstant`
* `NotAConstCommand`
* `NotAConstHelp`
* `InvalidGlobPattern`
* `ErrorExpandingGlob`

Fixes nushell#10700 

# User-Facing Changes

None

# Tests + Formatting

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed meta-issue An issue that tracks other issues unhelpful-error The error message you observe is not helpful to identify the problem
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants