Skip to content

Improve error when regex rejects pattern. Resolution of #8037#8050

Merged
fdncred merged 3 commits intonushell:mainfrom
bobhy:fix_8037
Feb 12, 2023
Merged

Improve error when regex rejects pattern. Resolution of #8037#8050
fdncred merged 3 commits intonushell:mainfrom
bobhy:fix_8037

Conversation

@bobhy
Copy link
Copy Markdown
Contributor

@bobhy bobhy commented Feb 12, 2023

Description

Improve error when str replace <pattern> detects a problem with .

User-Facing Changes

New "Incorrect value" error:

〉 'C:\Users\kubouch' | str replace 'C:\Users' 'foo'
Error: nu::shell::incorrect_value (link)

  × Incorrect value.
   ╭─[entry #1:1:1]
 1 │  'C:\Users\kubouch' | str replace 'C:\Users' 'foo'
   ·                                   ─────┬────
   ·                                        ╰── Regex error: Parsing error at position 4: Invalid hex escape
   ╰────

We could fruitfully replace some of the current uses of ShellError::UnsupportedInput with this error. 'Incorrect value' is different from 'wrong type'

Tests + Formatting

Don't forget to add tests that cover your changes.

  • none added / changed.

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.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Feb 12, 2023

nice research and fix. thanks!

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 12, 2023

Codecov Report

Base: 55.23% // Head: 54.86% // Decreases project coverage by -0.38% ⚠️

Coverage data is based on head (4e7f049) compared to base (9777d75).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8050      +/-   ##
==========================================
- Coverage   55.23%   54.86%   -0.38%     
==========================================
  Files         606      606              
  Lines       99065    99060       -5     
==========================================
- Hits        54720    54348     -372     
- Misses      44345    44712     +367     
Impacted Files Coverage Δ
crates/nu-command/src/strings/str_/replace.rs 93.40% <0.00%> (+2.31%) ⬆️
crates/nu-protocol/src/shell_error.rs 85.55% <ø> (ø)
crates/nu-color-config/src/nu_style.rs 9.31% <0.00%> (-70.92%) ⬇️
crates/nu-color-config/src/style_computer.rs 81.31% <0.00%> (+0.54%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Feb 12, 2023

Thanks! The error message makes it much more obvious what's wrong.

@fdncred fdncred added this pull request to the merge queue Feb 12, 2023
@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Feb 12, 2023

I'm checking out why the line is not covered by the test and it seems like the test is not being run at all. Maybe the str tests are disabled for some reason.

EDIT: Actually, I want the tests locally and the test is being run. It's just somehow it doesn't get included in the coverage report.

@sholderbach Do you have an idea why the test is not covered? Because it seems to exercise the code path. There is no other line that could return the IncorrectValue error which is what the test checks.

Merged via the queue into nushell:main with commit 2894668 Feb 12, 2023
Comment on lines +94 to +101
/// A command received an argument with correct type but incorrect value.
///
/// ## Resolution
///
/// Correct the argument value before passing it in or change the command.
#[error("Incorrect value.")]
#[diagnostic(code(nu::shell::incorrect_value), url(docsrs))]
IncorrectValue(String, #[label = "{0}"] Span),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be worth investigating if we already have similar variants that are poorly documented.

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.

4 participants