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

adds better error for failed string-to-duration conversions #5977

Merged
merged 2 commits into from Jul 7, 2022

Conversation

merelymyself
Copy link
Contributor

Description

Fixes #5975. New error can probably be used for other issues of a similar kind.

Tests

Make sure you've done the following:

  • Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
  • Try to think about corner cases and various ways how your changes could break. Cover them with tests.
  • If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.
/home/gabriel/CodingProjects/nushell〉echo [[value]; ['1.1ms'] ['12s'] ['2min'] ['3hr'] ['4day'] ['5wk']] | into duration value     07/07/2022 04:49:40 PM
Error: nu::shell::cant_convert_with_value (link)

  × Can't convert to duration.
   ╭─[entry #1:1:1]
 1 │ echo [[value]; ['1.1ms'] ['12s'] ['2min'] ['3hr'] ['4day'] ['5wk']] | into duration value
   ·                                                                       ──────┬──────
   ·                                                                             ╰── can't convert string `12s` to duration
   ╰────
  help: supported units are ns, us, ms, sec, min, hr, day, and wk

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

@WindSoilder
Copy link
Collaborator

WindSoilder commented Jul 7, 2022

What about making error message like this? It seems more intuitive to me

Error: nu::shell::cant_convert (link)

  × Can't convert to duration.
   ╭─[entry #2:1:1]
 1 │ echo [[value]; ['1.1ms'] ['12s'] ['2min'] ['3hr'] ['4day'] ['5wk']] | into duration value
   ·                           ──┬──
   ·                             ╰── can't convert string to duration
   ╰────
  help: supported units are ns, us, ms, sec, min, hr, day, and wk

@merelymyself
Copy link
Contributor Author

@WindSoilder - with the new commit, I guess I met you in the middle? I still think the operator should be labelled, in accordance with all other nushell errors, but your suggestion was good as well, so now both are labelled. And in case there's a labelling error, the error header still gives all the info necessary

/home/gabriel/CodingProjects/nushell〉echo [[value]; ['1.1ms'] ['12s'] ['2min'] ['3hr'] ['4day'] ['5wk']] | into duration value    07/07/2022 06:19:54 PM
Error: nu::shell::cant_convert_with_value (link)

  × Can't convert string `12s` to duration.
   ╭─[entry #1:1:1]
 1 │ echo [[value]; ['1.1ms'] ['12s'] ['2min'] ['3hr'] ['4day'] ['5wk']] | into duration value
   ·                           ──┬──                                       ──────┬──────
   ·                             │                                               ╰── can't be converted to duration
   ·                             ╰── this string value...
   ╰────
  help: supported units are ns, us, ms, sec, min, hr, day, and wk

@fdncred
Copy link
Collaborator

fdncred commented Jul 7, 2022

I don't see any tests but I'm not sure there is a way to add tests for this? If you think of one please add it. Thanks.

@fdncred fdncred merged commit 47f6d20 into nushell:main Jul 7, 2022
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.

better error reporting for errors encountered during iteration
3 participants