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

Treat empty pipelines as pass-through #8395

Merged
merged 3 commits into from Jun 18, 2023

Conversation

sophiajt
Copy link
Member

Description

This allows empty pipelines to pass their emptiness through a filter. This helps fix issues like trying to run a filter on an ls in an empty directory. It also feels a bit more reasonable that a filter filters what is there but doesn't require something to be there.

fixes #8393

User-Facing Changes

No breaking changes (that I know of). Should allow filtering to be a little less surprising with emptiness.

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.

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #8395 (b77bc45) into main (ca275f5) will decrease coverage by 0.08%.
The diff coverage is 50.00%.

❗ Current head b77bc45 differs from pull request most recent head 65f166c. Consider uploading reports for the commit 65f166c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8395      +/-   ##
==========================================
- Coverage   68.54%   68.46%   -0.08%     
==========================================
  Files         636      620      -16     
  Lines      102281    99520    -2761     
==========================================
- Hits        70110    68138    -1972     
+ Misses      32171    31382     -789     
Impacted Files Coverage Δ
crates/nu-protocol/src/pipeline_data.rs 77.99% <50.00%> (+2.48%) ⬆️

... and 264 files with indirect coverage changes

@sophiajt
Copy link
Member Author

I want to also land some corresponding fixes to some of the filter commands, so we have a consistent way filtering works with empty pipelines

@fdncred
Copy link
Collaborator

fdncred commented Apr 14, 2023

@jntrnr Thoughts on this one? We have time before the next release if we land soon.

@fdncred
Copy link
Collaborator

fdncred commented May 12, 2023

pinging you again @jntrnr so that, if we want this, we can land it shortly after the next release in a few days.

this should solve the merge conflicts in nushell#8395.
@amtoine
Copy link
Member

amtoine commented Jun 18, 2023

💡 #8393 has been fixed around the same time independently in #8439 👌

i think the two approaches are complementary and should be kept ✨

@amtoine amtoine merged commit 1d5e7b4 into nushell:main Jun 18, 2023
20 checks passed
@sholderbach sholderbach added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Jun 18, 2023
@sholderbach
Copy link
Member

Marking as a breaking changes as this could be a potential change to the semantics of erroneous usage of filtering commands after "void" commands that return PipelineData::Empty

@amtoine
Copy link
Member

amtoine commented Jun 18, 2023

@sholderbach
you're absolutely right, this is a breaking change, mainly on error handling

> cd . | where blah
Error: nu::shell::only_supports_this_input_type

  × Input type not supported.
   ╭─[entry #13:1:1]
 1 │ cd . | where blah
   ·        ──┬──┬
   ·          │  ╰── input type: null
   ·          ╰── only list, binary, raw data or range input data is supported
   ╰────

now becomes silent... 😕

> cd . | where blah
╭────────────╮
│ empty list │
╰────────────╯

@amtoine
Copy link
Member

amtoine commented Jun 18, 2023

if we want things like cd . | where blah to return an error (i think it should), we should probably revert that and add a test for this 🤔

@sophiajt sophiajt deleted the forgiving_strict_iter branch June 18, 2023 19:09
sholderbach added a commit that referenced this pull request Jun 18, 2023
sophiajt pushed a commit that referenced this pull request Jun 20, 2023
In my view we should revert #8395 for now

## Potentially inconsistent application of semantic change
#8395 (1d5e7b4) was loosening the type
coercion rules significantly, to let missing data / void returns that
were either expressed by `PipelineData::Empty` or the `Value::nothing`
be accept by specifically those commands/operations that made use of
`PipelineData::into_iter_strict()`. This could apply the new rules
inconsistently.

## Turning explicit failures into silent continuations
Furthermore the effect of this breaking change to the missing data
semantics could make previous errors into silent failures.
This could either just reduce the effectiveness of teaching error
messages in interactive use:

### Contrived example before
```bash
> cd . | where blah
Error: nu::shell::only_supports_this_input_type

  × Input type not supported.
   ╭─[entry #13:1:1]
 1 │ cd . | where blah
   ·        ──┬──┬
   ·          │  ╰── input type: null
   ·          ╰── only list, binary, raw data or range input data is supported
   ╰────
```
### ...after, with #8395
```bash
> cd . | where blah
╭────────────╮
│ empty list │
╰────────────╯
```

In rare cases people could already try to rely on catching an error of a
downstream command to actually deal with the missing data, so it would
be a breaking change for their existing code.

## Problem with `PipelineData::into_iter_strict()`

Maybe this makes `_strict` a bit of a misnomer for this particular
iterator construction.
Further we did not actively test the `PipelineData::empty` branch before

![grafik](https://github.com/nushell/nushell/assets/15833959/c377bf1d-d47c-4c25-a342-9a348539f242)

## Parsimonious solution exists

For the motivating issue #8393
there already exists a fix that makes `ls` more consistent with the type
system by returning an empty `Value::List`
#8439
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variable assignment fails due to piping no values in an empty directory
4 participants