Improve how trogon is calling the command with arguments #107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bigbirdcode
changed the title
|
I have tested this PR and it solve my issues with running commands on Windows. Especially #67. |
|
I am waiting on the merge as well |
|
@daneah by any chance can you review and comment my PR? |
|
I'll try to review in depth soon @bigbirdcode — I'm working through finalizing my talk about our internal CLI tooling for PyTexas this coming weekend. |
daneah
reviewed
Apr 16, 2025
| import sys | ||
| from types import ModuleType | ||
|
|
||
| import oslex |
There was a problem hiding this comment.
I'm not confident we should be taking on this dependency—it appears not to have been updated in some time, nor with many stars on GitHub. It is a small enough footprint that we should perhaps bring what we need of its functionality here into this package directly.
| @@ -84,7 +84,7 @@ def to_cli_args(self, include_root_command: bool = False) -> list[str]: | |||
| Returns: | |||
| A list of strings that can be passed to subprocess.run to execute the command. | |||
| """ | |||
| cli_args = self._to_cli_args() | |||
| cli_args = list(str(arg) for arg in self._to_cli_args()) | |||
There was a problem hiding this comment.
Suggested change
| cli_args = list(str(arg) for arg in self._to_cli_args()) | |
| cli_args = [str(arg) for arg in self._to_cli_args()] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to resolve several problems. All the changes are small and connected, that is why I opened just one PR.
Please review it commit by commit.
Fixes:
#104 - In
run_command.pyfile theUserCommandData.to_cli_args()now contains string conversion.I have not added conversion to
_to_cli_args()as it was already too complex.#105 -
shlexreplaced tooslex, for the need of Windows users.I have added to
pyproject.toml.TODO:
poetry.lockwas not updated, I don't have tooling for that.#106 - To run well and wait for the output on Windows I have added
sys.exit(subprocess.call(..., shell=True))as the pair ofos.execvp()call.Note:
shell=Truewas needed for me.#81 , #70 , #67 , #65 - I try to fix all these. In the file
detect_run_string.pythe functiondetect_run_string()returns a short version of the command, while for execution we need the full version. So I have added a functionexact_run_commands(). I have also modified how the command is called introgon.pyand replacedos.execvp()toos.execv().Tested:
-m, outside of a venv with direct path to Python.exe, and with installed app-m, outside of a venv with direct path to python3I have not tested with pipx or such cases. Please tell me of more modification is needed.