Skip to content
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

Various improvements to run.py #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jchorl
Copy link

@jchorl jchorl commented Sep 22, 2024

I'd recommend reviewing commit-by-commit, I tried to keep them isolated and understandable. In order:

  1. Switch from passing concatenated strings to subprocess.check_output to passing lists to subprocess.run. The reason is that "my_cool_app " + input_file will break if input_file contains a space or a myriad of other special chars. This can also enable vulnerabilities if e.g. a file is named || true; rm -rf /
  2. Adopt pathlib.Path for cwd, it's easier to do path operations and seems to be a successor for os.path
  3. Similarly, type args with Path and int and bool. Might as well rely on argparse to properly parse types.
  4. Just removing an unnecessary comment
  5. I ran the ruff linter and got it passing on run.py. I noticed mixing 2 vs. 4 space indentation, some unused vars, etc. The linter put the code in better shape.

I only got to run.py as some of the other files are bigger and more work. But hopefully this helps. Tested on python3.12.

@jchorl
Copy link
Author

jchorl commented Sep 22, 2024

Note there are a couple breaking changes here:

  • -k/--keep uses BooleanOptionalAction, so the flags become -k/--keep and --no-keep (as opposed to -k true or -k false)
  • Same for -r/--repeats

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant