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

Type validation for headers command (#6918) #7047

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

raccmonteiro
Copy link
Contributor

@raccmonteiro raccmonteiro commented Nov 7, 2022

Description

Type validation for headers command. We are only allowed to view the following types in headers: Null, String, Bool, Float, Int. Any other type different than these ones will result in an error like the image below.

New error message: "needs compatible type: Null, String, Bool, Float, Int"

image

Fixes #6918

Tests + Formatting

Commands with error:

  • [[a b]; [1 ($nu.scope.modules)] [2 2]] | headers
  • [[a b]; [{}, 2], [3,4] ] | headers
  • [[a b]; [[f,g], 2], [3,4] ] | headers

Valid commands:

  • "a b c|1 2 3" | split row "|" | split column " " | headers
  • [[a b]; [false, 2], [3,4] ] | headers
  • [[a b]; [1, 2], [3,4] ] | headers
  • [[a b]; [null, 2], [3,4] ] | headers

User-Facing Changes

No major change.

@rgwood
Copy link
Contributor

rgwood commented Nov 7, 2022

Thanks for the PR. I think this makes sense.

Could you please add some tests? I think you could convert some of your "Commands with error" examples into tests that expect an error.

cargo clippy lints

tests

format
@raccmonteiro
Copy link
Contributor Author

@rgwood I added some tests like you suggested.

@raccmonteiro raccmonteiro changed the title Type validation for headers command (#6918) Type validation for headers command (#6918) Nov 8, 2022
@rgwood
Copy link
Contributor

rgwood commented Nov 10, 2022

Thank you - LGTM!

@rgwood rgwood merged commit 7b0c069 into nushell:main Nov 10, 2022
@raccmonteiro raccmonteiro deleted the headers-type-validation branch December 1, 2022 20:19
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.

headers should error instead of trying to coerce complicated values to string
2 participants