Windows support for Lydia docker #60
Conversation
| @@ -32,7 +32,7 @@ | |||
|
|
|||
| def call_lydia(*args) -> str: | |||
| """Call the Lydia CLI tool with the arguments provided.""" | |||
| command = ["lydia", *args] | |||
| command = ["lydia" if sys.platform == "win32" else "lydia.bat", *args] | |||
There was a problem hiding this comment.
is it possible to avoid the .bat extension on Windows, and still make the script foundable by the OS?
There was a problem hiding this comment.
This would simplify the code as it would not need the if condition.
There was a problem hiding this comment.
Moreover, sys not imported! Please address.
There was a problem hiding this comment.
Added the sys import and corrected the check.
Regarding the file extension, Windows executes by default .bats, .coms and .exes. These extension are in the PATHEXT environment variable. The proposed solution requires the least configuration by the user and introduces just one if condition, which I believe is a good tradeoff.
There was a problem hiding this comment.
Thanks a lot!
Just a comment. We adopt the "Git Workflow" convention to make PR on develop rather than main, as in main only release commits are allowed. This change will be added for the next release.
Once merged on branch whitemech:develop, the package can then be installed via:
pip install git+https://github.com/whitemech/logaut.git@develop#egg=logaut
|
|
|
@gallorob please, can you also run |
Co-authored-by: Marco Favorito <marco.favorito@gmail.com>
|
Merging since I cannot do changes directly on |
Proposed changes
Workaround to find the Lydia docker from Windows' IDEs (such as Pycharm).
Fixes
Fixes the FileNotFound error when looking for the Lydia executable.
Types of changes
Checklist
Further comments
Encountered bug while working on the exam's project.