Skip to content

Commit

Permalink
Swap additional context and label in I/O errors (nushell#14954)
Browse files Browse the repository at this point in the history
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

Tweaks the error style for I/O errors introduced nushell#14927. Moves the
additional context to below the text that says "I/O error", and always
shows the error kind in the label.

Additional context|Before PR|After PR
:-:|:-:|:-:

yes|![image](https://github.com/user-attachments/assets/df4f2e28-fdf5-4693-b60c-255d019af25f)
|
![image](https://github.com/user-attachments/assets/5915e9d0-78d4-49a6-b495-502d0c6444fa)
no|
![image](https://github.com/user-attachments/assets/e4ecaada-ec8c-4940-b08a-bbfaa45083d5)
|
![image](https://github.com/user-attachments/assets/467163d8-ab39-47f0-a74f-e2effe2fe6af)



# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

N/A, as this is a follow-up to nushell#14927 which has not been included in a
release

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->
N/A

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
N/A

---------

Co-authored-by: Piepmatz <[email protected]>
  • Loading branch information
132ikl and cptpiepmatz authored Feb 3, 2025
1 parent 30b3c42 commit f04db2a
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 17 deletions.
17 changes: 10 additions & 7 deletions crates/nu-command/src/filesystem/open.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,16 @@ impl Command for Open {
#[cfg(unix)]
let err = {
let mut err = err;
err.additional_context = Some(match path.metadata() {
Ok(md) => format!(
"The permissions of {:o} does not allow access for this user",
md.permissions().mode() & 0o0777
),
Err(e) => e.to_string(),
});
err.additional_context = Some(
match path.metadata() {
Ok(md) => format!(
"The permissions of {:o} does not allow access for this user",
md.permissions().mode() & 0o0777
),
Err(e) => e.to_string(),
}
.into(),
);
err
};

Expand Down
28 changes: 18 additions & 10 deletions crates/nu-protocol/src/errors/shell_error/io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ pub struct IoError {
///
/// Only set this field if it adds meaningful context.
/// If [`ErrorKind`] already contains all the necessary information, leave this as [`None`].
pub additional_context: Option<String>,
pub additional_context: Option<AdditionalContext>,

/// The precise location in the Rust code where the error originated.
///
Expand All @@ -139,6 +139,16 @@ pub enum ErrorKind {
IsADirectory,
}

#[derive(Debug, Clone, PartialEq, Eq, Error, Diagnostic)]
#[error("{0}")]
pub struct AdditionalContext(String);

impl From<String> for AdditionalContext {
fn from(value: String) -> Self {
AdditionalContext(value)
}
}

impl IoError {
/// Creates a new [`IoError`] with the given kind, span, and optional path.
///
Expand Down Expand Up @@ -209,7 +219,7 @@ impl IoError {
kind: kind.into(),
span,
path,
additional_context: Some(additional_context.to_string()),
additional_context: Some(additional_context.to_string().into()),
location: None,
}
}
Expand Down Expand Up @@ -249,7 +259,7 @@ impl IoError {
kind: kind.into(),
span: Span::unknown(),
path: None,
additional_context: Some(additional_context.to_string()),
additional_context: Some(additional_context.to_string().into()),
location: Some(location.to_string()),
}
}
Expand Down Expand Up @@ -283,7 +293,7 @@ impl IoError {
kind: kind.into(),
span: Span::unknown(),
path: path.into(),
additional_context: Some(additional_context.to_string()),
additional_context: Some(additional_context.to_string().into()),
location: Some(location.to_string()),
}
}
Expand Down Expand Up @@ -370,16 +380,14 @@ impl Diagnostic for IoError {
(true, Some(location)) => SourceSpan::new(0.into(), location.len()),
};

let label = match self.additional_context.as_ref() {
Some(ctx) => format!("{ctx}\n{}", self.kind),
None => self.kind.to_string(),
};
let label = LabeledSpan::new_with_span(Some(label), span);
let label = LabeledSpan::new_with_span(Some(self.kind.to_string()), span);
Some(Box::new(std::iter::once(label)))
}

fn diagnostic_source(&self) -> Option<&dyn Diagnostic> {
Some(&self.kind as &dyn Diagnostic)
self.additional_context
.as_ref()
.map(|ctx| ctx as &dyn Diagnostic)
}

fn source_code(&self) -> Option<&dyn miette::SourceCode> {
Expand Down

0 comments on commit f04db2a

Please sign in to comment.