-
Notifications
You must be signed in to change notification settings - Fork 11
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
stdin works on Windows, too, without regressing issue #5 #13
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 242 254 +12
=========================================
+ Hits 242 254 +12 ☔ View full report in Codecov by Sentry. |
Im currently on a vacation for the next few days, so i will do a CR later in the week :) |
Seems good to me, Can you fix the coverage to 100%? |
I added windows-latest to the github actions matrix. I'm not really familiar with github actions, so please let me know if more is needed. |
It seems to work, but there is a problem in the make file with windows, can you fix it too? |
No problem. I'll look at the makefile and test coverage tomorrow.
…--
Jeff Shipley
On Tue, Feb 13, 2024, at 10:58 PM, roy reznik wrote:
It seems to work, but there is a problem in the make file with windows, can you fix it too?
Also please add test that will make it still 100% coverage.
You can tedt the coverage using the 'make test' command and change the coverage parameter to html
—
Reply to this email directly, view it on GitHub <#13 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFADI2POEIEAWEF33YE73YTRHBRAVCNFSM6AAAAABDDIKUP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBTGEZTCOBTGU>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Also removed '-rf' from rm command to fix windows matrix tests
I pushed in another change. For the matrix test, I think that removing "-rf" from the "rm -rf" command should be enough to get it to work. The output from the build makes it look like powershell is running the Windows commands. In powershell, "rm" works, but it runs the "Remove-Item" cmdlet that has different flags than the Linux rm command. It looks like poetry.lock is a file, not a directory, so the "-rf" was unnecessary. If for some reason this doesn't work, it'll probably need a condition to make it run a different rm command on Windows and Linux. I ended up writing a simple test case for the new isatty wrapper function, because no matter what I try I can't get a mock for sys.stdin.isatty to work inside the rexi_cli typer function. It might be because of the interaction with typer, or it might be the reassignment of sys.stdin that's happening. Long story short, test coverage should be 100% now, and the matrix tests should work. |
Hmm. It looks like it's failing now because make isn't available on the Windows runner. I'll add a step to install make, but I don't know of any way to test running a GitHub action without actually checking in the changes and letting GitHub run it. |
Im going to push a ccommit that might fix it here |
For now lets remove the github action for windows. |
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.
Looks good
This fixes #12
This makes rexi work on Windows again, and is also a good fix for issue #5 where rexi was waiting for input forever when no input was given.
I have run the unit tests and other checks on both Windows and Linux, and everything seems to be working as expected.
I'm not going to guarantee that it will work on any version of Windows older than Windows 10, but given that Windows 10 is EOL in about a year and a half that shouldn't matter.