-
Notifications
You must be signed in to change notification settings - Fork 10
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
Handle the rewrite of "-" to "/dev/stdin" in main to leave the filenames unchanged (fixes #46) #47
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
- Coverage 76.98% 76.86% -0.12%
==========================================
Files 9 9
Lines 2572 2576 +4
Branches 659 660 +1
==========================================
Hits 1980 1980
- Misses 462 466 +4
Partials 130 130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ac1d824
to
8a4e3d1
Compare
I marked this PR as draft because reading "/dev/stdin" doesn't work on Windows, as was to be expected. Let's fix this while working on this code. |
00a63af
to
84ad116
Compare
from: os("/dev/stdin"), | ||
to: os("/dev/stdin"), | ||
from: os("-"), | ||
to: os("-"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep at least one test that uses /dev/stdin
. I think the right way to mark a test as "unix-only" is #[cfg(unix)]
right before fn
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those tests do not actually exercise reading from stdin, only the parsing of command-line parameters. I don't see much added value to verifying that /dev/stdin
doesn't get rewritten.
However I can add an integration test that exercises reading from /dev/stdin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I got that wrong. And thank you, that's exactly what I was hoping for! :)
No description provided.