Skip to content

Fix ignore-errors for select#6896

Merged
sophiajt merged 8 commits intonushell:mainfrom
nibon7:fix_select
Nov 9, 2022
Merged

Fix ignore-errors for select#6896
sophiajt merged 8 commits intonushell:mainfrom
nibon7:fix_select

Conversation

@nibon7
Copy link
Copy Markdown
Contributor

@nibon7 nibon7 commented Oct 25, 2022 •
edited
Loading

Description

currently, the ignore-errors flag for select doesn't work well, hope this PR fixes this.

  • before
    Screenshot from 2022-10-25 22-38-18
    Screenshot from 2022-10-25 22-38-49
    Screenshot from 2022-10-25 22-38-38
    Screenshot from 2022-10-25 23-15-38

  • after
    Screenshot from 2022-10-25 22-40-08
    Screenshot from 2022-10-25 23-16-26

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.

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

Documentation

@sholderbach
Copy link
Copy Markdown
Member

sholderbach commented Oct 25, 2022

Can we put together a small test to not regress on that behavior?

@sholderbach
Copy link
Copy Markdown
Member

sholderbach commented Oct 25, 2022

Thanks for addressing that. Your code looks much more readable than before!

@nibon7
Copy link
Copy Markdown
Contributor Author

nibon7 commented Oct 25, 2022 •
edited
Loading

I think the tests should cover all the changes :)

@sholderbach sholderbach added the notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes label Oct 27, 2022
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 30, 2022

So, if I understand this properly, the idea of select -i is to fail silently, correct? If so, I think this is working better now and we should land it.

I tested this pr with this ls | select -I created as well as the other examples above and they all work as stated.

@nibon7 the last thing that I have a question about is the help text, as I think it is incorrect now. Please confirm.

-i, --ignore-errors - when a column has empty cells, instead of erroring out, replace them with nothing

I don't think "replace them with nothing" is right now.

@nibon7
Copy link
Copy Markdown
Contributor Author

nibon7 commented Oct 30, 2022 •
edited
Loading

So, if I understand this properly, the idea of select -i is to fail silently, correct?

exactly.

I don't think "replace them with nothing" is right now.

I think the current implementation just silences the errors, and "when a column has empty cells, instead of erroring out, replace them with nothing" is inaccurate.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 30, 2022

Right. We need to tweak that. After that change I'll vote to land this PR.

fdncred
fdncred approved these changes Oct 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

I'm fine with these changes.

@sophiajt sophiajt merged commit c600c1e into nushell:main Nov 9, 2022
@nibon7 nibon7 deleted the fix_select branch November 10, 2022 00:38
@Decodetalkers Decodetalkers mentioned this pull request Dec 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fdncred fdncred fdncred approved these changes

Assignees

No one assigned

Labels

notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nibon7 @sholderbach @fdncred @sophiajt