Skip to content

Error out when Select gets same row#8200

Merged
rgwood merged 2 commits intonushell:mainfrom
dmatos2012:select-same-index-twice-hangs
Feb 27, 2023
Merged

Error out when Select gets same row#8200
rgwood merged 2 commits intonushell:mainfrom
dmatos2012:select-same-index-twice-hangs

Conversation

@dmatos2012
Copy link
Copy Markdown
Contributor

@dmatos2012 dmatos2012 commented Feb 24, 2023

Description

Fixes #8145, by disallowing any rows that are duplicated.

❯ ls | select 0 0
Error: 
  × Select only allows unique rows
   ╭─[entry #1:1:1]
 1 │ ls | select 0 0
   ·               ┬
   ·               ╰── duplicated row
   ╰────

User-Facing Changes

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.

@sophiajt
Copy link
Copy Markdown
Contributor

sophiajt commented Feb 24, 2023

I wonder if we should check the indexes they ask for, and if they are the same ones, error there and say that you can't pass the same index twice.

It'd be okay for two rows with the same values to be returned.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 25, 2023 •
edited
Loading

Codecov Report

Merging #8200 (bec3a0b) into main (42f0b55) will increase coverage by 13.61%.
The diff coverage is 57.70%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #8200       +/-   ##
===========================================
+ Coverage   54.77%   68.39%   +13.61%     
===========================================
  Files         613      617        +4     
  Lines       98494    98991      +497     
===========================================
+ Hits        53954    67705    +13751     
+ Misses      44540    31286    -13254
Impacted Files Coverage Δ
crates/nu-engine/src/eval.rs 75.00% <0.00%> (+22.04%) ⬆️
...ates/nu-cli/src/completions/command_completions.rs 81.67% <95.00%> (+81.67%) ⬆️
crates/nu-cli/src/completions/completer.rs 89.21% <96.07%> (+89.21%) ⬆️
crates/nu-command/src/filters/select.rs 90.87% <100.00%> (+10.26%) ⬆️
crates/nu-protocol/src/engine/engine_state.rs 85.29% <100.00%> (+22.93%) ⬆️
crates/nu-engine/src/nu_variable.rs 27.94% <0.00%> (-0.42%) ⬇️
crates/nu-command/src/strings/str_/trim/trim_.rs 97.44% <0.00%> (-0.32%) ⬇️
crates/nu-command/src/bytes/starts_with.rs 88.33% <0.00%> (-0.18%) ⬇️
crates/nu-command/src/env/config/utils.rs 0.00% <0.00%> (ø)
crates/nu_plugin_gstat/src/lib.rs 100.00% <0.00%> (ø)
... and 259 more

@dmatos2012
Copy link
Copy Markdown
Contributor Author

dmatos2012 commented Feb 25, 2023 •
edited
Loading

I wonder if we should check the indexes they ask for, and if they are the same ones, error there and say that you can't pass the same index twice.

Im maybe a bit confused, but I thought that was what I was doing. The moment its repeated, it errors out. Or perhaps you mean dealing with the indices in a different part of the code?

rgwood
rgwood approved these changes Feb 27, 2023
View reviewed changes
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.

This looks good to me, thank you for the PR!

I agree that your code is doing what JT asked for. I've tweaked the error message wording to make that a little more clear.

@rgwood rgwood merged commit 96e3a3d into nushell:main Feb 27, 2023
@dmatos2012 dmatos2012 deleted the select-same-index-twice-hangs branch October 24, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rgwood rgwood rgwood approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

select 0 0 hangs Nushell

3 participants

@dmatos2012 @sophiajt @rgwood