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

fix: stdin behavior #1336

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tessus
Copy link

@tessus tessus commented Jan 18, 2025

The issue is that eza ignores the --stdin argument, unless there is data piped into it. This is not how stdin behavior is supposed to work. e.g. look at the cat command.
The flag should tell eza that the input is read from stdin. But if you call eza --stdin nothing is read from stdin.

This means that the current argument is moot and serves no purpose, because eza already has the capability of checking whether data is piped into it.
So eza behaves like this: if you pipe data into eza, eza will ignore the data, unless you additionally specify --stdin
This makes no sense.
Besides, what else is eza supposed to read when data is piped into it?

closes #1326

@tessus tessus requested a review from MartinFillon as a code owner January 18, 2025 20:43
@gierens
Copy link
Member

gierens commented Jan 20, 2025

FreeBSD tests are failing due to clippy issues that this PR among other things fixes: #1327

@tessus
Copy link
Author

tessus commented Jan 20, 2025

That's what I thought at first (and opened #1335), but I was wrong. This pipeline fails because of an rsync issue.

@gierens
Copy link
Member

gierens commented Jan 20, 2025

That's what I thought at first (and opened #1335), but I was wrong. This pipeline fails because of an rsync issue.

Ok, can you elaborate why you think this has to do with rsync?

@gierens
Copy link
Member

gierens commented Jan 20, 2025

That's what I thought at first (and opened #1335), but I was wrong. This pipeline fails because of an rsync issue.

Maybe I'm missing something but from what I can see, clippy fails in the "Run 'run' in VM" step:
Screenshot From 2025-01-20 10-46-31

And yes, as you have pointed out in the other issue this due to the FreeBSD tests using the latest version available (currently 1.83.0) and just for confirmation, I'm getting the same response locally when trying to compile with 1.83.0:
Screenshot From 2025-01-20 10-43-21

Since we are testing on Linux with different versions as well, I don't necessarily see this as a problem more of an early warning system of things we should fix for future rust versions.

@tessus
Copy link
Author

tessus commented Jan 20, 2025

@tessus
Copy link
Author

tessus commented Jan 20, 2025

Hmm, this is messed up. The error should show up in that step then.

Whatever, I'll wait until the other PR is merged and do a rebase.

@gierens
Copy link
Member

gierens commented Jan 20, 2025

Hmm, this is messed up. The error should show up in that step then.

Yeah that confused me at first, too ... The actual failure can be seen here: https://github.com/eza-community/eza/actions/runs/12847289727/job/35823428447#step:3:2475

I guess this not showing up correctly in the UI is due nesting from vmactions/freebsd-vm ... the BSD tests are all not running directly inside a Github CI container, but in a BSD VM inside a Ubuntu container since Github doesn't provide BSD CI if I remember correctly.

@gierens
Copy link
Member

gierens commented Jan 20, 2025

Btw. I'm currently trying to resolve the version mismatch in the BSD CI, see #1337 ... This should fix it as well

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

This is nice (and a clear improvement), but it is a breaking change, so before merge, could you add a breaking change footer to the commit and a !. If you're unsure how, it's explained in https://www.conventionalcommits.org/en/v1.0.0/, but if you have any questions, please ask!

@cafkafk cafkafk force-pushed the fix/stdin-behavior branch from cce9ec8 to 2bc74a8 Compare January 20, 2025 15:35
@tessus
Copy link
Author

tessus commented Jan 20, 2025

@cafkafk may I ask why this is a breaking change? I haven't removed --stdin.

The only difference is that when you invoke eza --stdin, it reads from standard input as described in the help.
I am more than happy to add such a notice in the commit message, I just need to know what the breaking change is, because I don't see it.

Also, many people (I usually do this) put BREAKING in the 1st line of the commit message as well , not only in the footer. According to conventional commits this is valid. Or don't you like it this way? However, first I really need to know what the breaking change is, otherwise I can't describe it in the commit message and footer.

@cafkafk
Copy link
Member

cafkafk commented Jan 21, 2025

@cafkafk may I ask why this is a breaking change?

The behavior of input piped into eza has changed.

@cafkafk
Copy link
Member

cafkafk commented Jan 21, 2025

Also, many people (I usually do this) put BREAKING in the 1st line of the commit message as well , not only in the footer. According to conventional commits this is valid. Or don't you like it this way? However, first I really need to know what the breaking change is, otherwise I can't describe it in the commit message and footer.

It's fine to put it in the first line of the message, would likely make it easier to spot, which makes a lot of sense. The reason for doing it the other way is to conform to the git idea of trailers https://git-scm.com/docs/git-interpret-trailers

Copy link
Contributor

@MartinFillon MartinFillon left a comment

Choose a reason for hiding this comment

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

This is a good behavior change. It is indeed a good idea.

@tessus
Copy link
Author

tessus commented Jan 21, 2025

The behavior of input piped into eza has changed.

I don't understand. How has it changed?

If data is piped into eza, eza will still work the same way people have been using it until now. (flocate something |eza -l --stdin)
A breaking change means that a previous intended behavior is not working anymore.
But it does and still will.

It would have only been a breaking change, if I had removed the --stdin flag.

The fact that --stdin now works, without piping data into eza, is a fix.
The fact that one does not need --stdin, when data is piped into eza, is a feature that more or less resulted from the fix.
Neither of those 2 behavior changes are breaking either.

I am sorry, I really do not understand why this is a breaking change.

P.S.: Please note, I don't want to be difficult, but I seriously don't understand how this is a breaking change. I kindly ask you to explain to me which behavior will no longer work (e.g. break a user's script or interactive command invocation) after this PR has been merged.

@cafkafk
Copy link
Member

cafkafk commented Jan 22, 2025

Thanks for clarifying that you're genuine and not trying to be difficult. Consider that, for instance, the behavior has changed here:

echo "./flake.nix" | oldeza -l
echo "./flake.nix" | neweza -l

One will list the entire directory, the other will only list the ./flake.nix. It may be the case that we have never guaranteed that behavior, but that said, experience shows that regardless, people will come to rely on it. The primary usage of the semantic version in eza is to indicate that something may break, we don't put as much marketing into the idea of moving up a version, and so leaning towards a cautious "something behaviorally is different than it was before means it's breaking" ensures we provide users with honestly one of the most stable cli experiences of most projects, and that's worth continuing.

It costs very little for us to make a breaking change here, and given that a previous behavior has changed, it is only right that our versioning reflects that.

@cafkafk cafkafk force-pushed the fix/stdin-behavior branch from 2bc74a8 to a3ba3e8 Compare January 22, 2025 11:55
@tessus
Copy link
Author

tessus commented Jan 22, 2025

Ok, I see. But... Who in their right mind pipes data into a program, expecting it to be ignored?

Why would someone write code like echo "bla" |eza -l if it is the same as eza -l? People like that deserve their scripts to fail in the first place. I doubt that this change will break their already broken script.

But ok, I admit that if you argue this insanity is normal and a dev has to expect that people rely on a behavior that makes no sense, it can be seen as a breaking change. However, I also mentioned earlier that no intended behavior is broken by this change.
But I give up. I'll mark it as breaking in the footer and will have to figure out an explanation in the commit message that doesn't make me look like an idiot.

I'll do that later today. Currently the fire department is conducting the annual inapection of the in-suite sirens and smoke detectors in my apartment building, and the blaring siren going off every minute is melting my brain.

The issue is that eza ignores the `--stdin` argument, unless there is
data piped into it. This is not how stdin behavior is supposed to work.
e.g. look at the `cat` command.
The flag should tell eza that the input is read from stdin. But if you
call `eza --stdin` nothing is read from stdin.

This means that the current argument is moot and serves no purpose,
because eza already has the capability of checking whether data is
piped into it.
So eza behaves like this: if you pipe data into eza, eza will ignore
the data, unless you additionally specify `--stdin`
This makes no sense.

This change accomplishes the following:

- `eza --stdin` reads from standard input
- if data is piped into eza, standard input is automatically read
  and no `--stdin` flag is required
- if data is piped into eza, `--stdin` can still be used

BREAKING CHANGE: if data is piped into eza, eza will not ignore it
@tessus tessus force-pushed the fix/stdin-behavior branch from a3ba3e8 to 0339e3a Compare January 22, 2025 21:53
@tessus
Copy link
Author

tessus commented Jan 22, 2025

I hope the commit message is ok now. If not, please make a suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

feat: a shorter alternative for "--stdin"
4 participants