Skip to content

#8027 Hide implementation details in invalid cd call#8049

Merged
rgwood merged 2 commits intonushell:mainfrom
ryand67:fs/better-cd-error-message-8027
Feb 13, 2023
Merged

#8027 Hide implementation details in invalid cd call#8049
rgwood merged 2 commits intonushell:mainfrom
ryand67:fs/better-cd-error-message-8027

Conversation

@ryand67
Copy link
Copy Markdown
Contributor

@ryand67 ryand67 commented Feb 12, 2023

Description

GH issue

The current error message for a cd command includes a Debug output of the io::Error being returned from canonicalize_with, so it's been replaced with a more user friendly and readable depiction of the error.

User-Facing Changes

As described in the issue, I've changed the error message for a cd into a directory that does not exist from:

/home/rdevenney/projects/open_source/nushell〉cd asdfasdf                                                                               02/11/2023 08:59:59 PM
Error: nu::shell::directory_not_found (link)

 × Directory not found
   ╭─[entry #2:1:1]
 1 │ cd asdfasdf
   ·    ────┬───
   ·        ╰── directory not found
   ╰────
  help: IO Error: Os { code: 2, kind: NotFound, message: "No such file or directory" }

To:

/home/rdevenney/projects/open_source/nushell〉cd asdfasdf                                                                               02/11/2023 08:58:38 PM
Error: nu::shell::directory_not_found (link)

  × Directory not found
   ╭─[entry #1:1:1]
 1 │ cd asdfasdf
   ·    ────┬───
   ·        ╰── directory not found
   ╰────
  help: IO Error: DirectoryNotFound

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 -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 12, 2023

Codecov Report

Merging #8049 (848d42c) into main (9777d75) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 848d42c differs from pull request most recent head 483f460. Consider uploading reports for the commit 483f460 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8049   +/-   ##
=======================================
  Coverage   55.23%   55.23%           
=======================================
  Files         606      606           
  Lines       99065    99065           
=======================================
+ Hits        54720    54723    +3     
+ Misses      44345    44342    -3     
Impacted Files Coverage Δ
crates/nu-command/src/filesystem/cd.rs 18.86% <0.00%> (ø)
crates/nu-color-config/src/style_computer.rs 81.31% <0.00%> (+0.54%) ⬆️
crates/nu-command/src/filesystem/touch.rs 64.76% <0.00%> (+1.03%) ⬆️

@ryand67 ryand67 force-pushed the fs/better-cd-error-message-8027 branch from 848d42c to d4bf80e Compare February 13, 2023 01:37
Copy link
Copy Markdown
Contributor

@rgwood rgwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the first PR!

I've tweaked it a tiny bit; I don't think we need the help: IO Error ... line at all.

@rgwood rgwood added this pull request to the merge queue Feb 13, 2023
Merged via the queue into nushell:main with commit 072d2a9 Feb 13, 2023
@rgwood rgwood linked an issue Feb 13, 2023 that may be closed by this pull request
@ryand67
Copy link
Copy Markdown
Contributor Author

ryand67 commented Feb 13, 2023

I was thinking the same, but didn’t want to be too presumptuous hahaha. Thank you! I look forward to being more of a part of this project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Confusing error message when cd to non-existing directory

2 participants