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

Add ability to take glob patterns from stdin #114

Merged
merged 5 commits into from
Apr 7, 2023

Conversation

jhscheer
Copy link
Contributor

@jhscheer jhscheer commented Apr 3, 2023

as requested in issue #51

This detects input on stdin using the is_terminal crate.
Each input line is treated as a separate glob pattern.

This adds a new (hidden) flag --stdin to indicate that et should
take input from stdin. Since the naming convention is to use - for
this, we replace the args - with --stdin before we hand them over
to Clap.

Example usage:

$ mkdir Hello; cd Hello
$ touch hello.rs helLo.rs HELlo.rs hello.txt
$  fd -e rs '^hel[lL]o' | et
Hello (2 B)
├─ hello.rs (1 B)
└─ helLo.rs (1 B)

jhscheer added 2 commits April 3, 2023 13:47
as requested in issue solidiquis#51

This adds a new (hidden) flag `--stdin` to indicate that `et` should
take input from stdin. Since the naming convention is to use `-` for
this, we replace the args `-` with `--stdin` before we hand them over
to Clap.

Example usage:
```shell
$ touch hello.rs helLo.rs HELlo.rs
$  fd -e rs '^hel[lL]o' | et -
           erdtree
     0   B ├─ hello.rs
     0   B └─ helLo.rs
```
@solidiquis
Copy link
Owner

Looks like we've got some failed tests. Additionally for this sort of thing I think it would be nice if we can have users pipe the things to et without having to manually allow reading from stdin. So like: fd -e rs '^hel[lL]o' | et

To allow for this sort of thing we'll need to check if stdin is a tty using this crate. If it isn't we proceed to traverse and treeify the output. Let me know what you think.

@jhscheer
Copy link
Contributor Author

jhscheer commented Apr 4, 2023

Fixed the test.

I can implement the isatty functionality in a follow-up PR.
However, I would argue in favor of is-terminal because it is an actively maintained port of atty and it is also a dependency of clap, which makes it already a part of our dependency graph.

@solidiquis
Copy link
Owner

I wasn't aware of that crate. Agreed, is-terminal should be preferred over atty.

Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

Let me know if there's anything you disagree with :]

src/main.rs Outdated Show resolved Hide resolved
src/render/context/mod.rs Outdated Show resolved Hide resolved
src/render/tree/node/mod.rs Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

Overall this looks fantastic, thank you for implementing this! My last request will be to remove the usage of - as per my other comment. Once that's done I'll get this merged in and include it in this Sunday's minor release :]

@solidiquis
Copy link
Owner

Hmmm any idea why this is happening? The test seems to be stuck. Happened yesterday as well I can help investigate if you need.

Screen Shot 2023-04-05 at 10 31 35 AM

@jhscheer
Copy link
Contributor Author

jhscheer commented Apr 5, 2023

I restarted the test, but it looks like it is stuck again.
I think it's an issue with github's CI, I doubt it's related to the changes in this PR.

Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

Okay so after doing some digging and playing around I've learned that Github Actions doesn't provide a tty so !stdin().is_terminal() evals to true which causes stdin to hang waiting for input. The solution is quite simple, we just need to modify tests/utils/mod.rs to be this:

    let output = cmd
        .stdin(Stdio::null()) // add this here
        .stdout(Stdio::piped())
        .stderr(Stdio::piped())
        .spawn()
        .unwrap()
        .wait_with_output()
        .unwrap();

Could you make that change and run tests again? Excited to get this merged in!

tests/utils/mod.rs Outdated Show resolved Hide resolved
* remove (hidden) flag `--stdin`
* remove ability to detect stdin with "-"
* refactor move stdin detection to Context::init
Copy link
Owner

@solidiquis solidiquis left a comment

Choose a reason for hiding this comment

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

Woot we are good! Thanks for doing this! 💪

@solidiquis solidiquis merged commit 285e7b3 into solidiquis:master Apr 7, 2023
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.

2 participants