royreznik / rexi

Terminal UI for Regex Testing
MIT License
312 stars 10 forks source link

stdin works on Windows, too, without regressing issue #5 #13

Closed jshipley closed 8 months ago

jshipley commented 9 months ago

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.

codecov-commenter commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (aefcbac) 100.00% compared to head (441b086) 100.00%. Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #13 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 6 6 Lines 242 254 +12 ========================================= + Hits 242 254 +12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

royreznik commented 9 months ago

Im currently on a vacation for the next few days, so i will do a CR later in the week :)

royreznik commented 8 months ago

Seems good to me, Can you fix the coverage to 100%? Also Can you add a Windows to the Actions Metrix? if you aren't familiar with it I will add it in another PR

jshipley commented 8 months ago

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.

royreznik commented 8 months ago

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

jshipley commented 8 months ago

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 https://github.com/royreznik/rexi/pull/13#issuecomment-1943131835, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAFADI2POEIEAWEF33YE73YTRHBRAVCNFSM6AAAAABDDIKUP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBTGEZTCOBTGU. You are receiving this because you authored the thread.Message ID: @.***>

jshipley commented 8 months ago

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.

jshipley commented 8 months ago

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.

royreznik commented 8 months ago

Im going to push a ccommit that might fix it here

royreznik commented 8 months ago

For now lets remove the github action for windows. It takes a lot of resources and i don't want to use all my action time for windows checking :D