midstar / pycstruct

A python library for reading and writing binary data similar to what is done in C language structs
MIT License
23 stars 4 forks source link

expose subprocess kwargs through parse_file call #41

Closed mhkline closed 1 year ago

mhkline commented 1 year ago

I would like to expose the subprocess kwargs because I'm having an issue with applications built by pyinstaller where subprocess calls cause console windows to pop-up and disappear. By exposing the subprocess kwargs, I can completely hide these windows.

Some details here: https://github.com/pyinstaller/pyinstaller/wiki/Recipe-subprocess

midstar commented 1 year ago

This looks like a good idea. Appveyor fails because you have not performed black formatting on the files prior commit.

mhkline commented 1 year ago

I ran black on the file I changed. Will now try the others as well.

mhkline commented 1 year ago

Not sure what is wrong here. Locally it is passing:

(venv-pyc) C:\Users\mkline\Code\pycstruct [expose-subprocess-kwargs]> black --check tests pycstruct
All done! ✨ 🍰 ✨
9 files would be left unchanged.

Am I missing something?

Edit: nevermind. seems it is the coverage report failing:

py38: commands[2]> coverage report --fail-under=89 pycstruct/cparser.py
Name                   Stmts   Miss  Cover
------------------------------------------
pycstruct/cparser.py     341     48    86%
py38: exit 2 (0.20 seconds) C:\Users\mkline\Code\pycstruct> coverage report --fail-under=89 pycstruct/cparser.py pid=35404

Edit 2: Seems I was wrong again. I did not have castxml installed.

The issue is that an update to pylint has triggered new warnings around exception messages being too broad.

image

Downgrading to 2.14.0 results in pylint passing.

mhkline commented 1 year ago

@midstar OK it is passing now. I decided to replace all instances of raise Exception() with raise RuntimeError(), as it is likely the most generic exception type that pylint will be happy about. Some of the errors could use even more specific exception (or even custom) types but not clear it's worth thinking too hard about it right now.

Alternative is to specify pylint==2.14.0 in tox.ini but I think it's better to fix these.

midstar commented 1 year ago

All changes are fine with me, but since you are updating the interface (by additional parameter and change of exception type) i would like you to modify version from 0.11.1 to 0.12.0 in setup.py and appveyor.yml. After this I will approve your change and merge it. .

mhkline commented 1 year ago

Done. BTW, thank you for creating this library. It's very nice for working with C!

midstar commented 1 year ago

Thank you very much for your contribution :-)