-
Notifications
You must be signed in to change notification settings - Fork 59
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
Code cleanup suggestions. #125
Labels
Comments
All good suggestions. Would you like me to review #93? I suppose we should keep / deprecate |
My PR #93 was flawed from the start since I tried to refactor the entire code base all at once. I assume I could refactor everything again over multiple distinct PR's. |
matthew-brett
added a commit
that referenced
this issue
Sep 22, 2021
MRG: Deprecate and replace calls to back_tick. Related to #125 The non-specific timeout check in `back_tick` has been removed. It was that or provide a specific timeout time. All internal functions and tests now use `subprocess.run`. This might change some of the exceptions raised by those functions except for `lipo_fuse` which was specifically tested to raise `RuntimeError`. Relevant tests have been cleaned up. Relevant functions have had type annotations added/updated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Some ideas that could make things easier for me and future maintainers:
I'd be nice to setup auto formatters like Black and isort. It's tedious to format all code manually.
doc/devel/make_release.rst should have been in a
CONTRIBUTING
file. I missed it entirely when I made the0.9.1
release. ACONTRIBUTING
file could also be used to explain how to add any test data.In #93 I attempted to fully type-hint the package. I never got that merged but it showed some areas that are causing issues such as
InWheelCtx.__enter__
andback_tick
. I thinkInWheelCtx
needs to be refactored or removed (or finish #46 and fix things there,) and all calls toback_tick
should be replaced withsubprocess.run
now that the code base is on Python 3.Testing has some issues. Everything from
pytest_tools.py
breaks the pytest debugger so they'll all eventually need to be replaced with plain asserts. It'd be nice to replacescriptrunner.py
with a more standard tool like pytest-console-scripts.Issue and PR templates might be a good idea. It sucks that most of the current bug reports don't mention which version of decloate they used and don't provide a wheel that can be checked.
CONTRIBUTING
file.InWheelCtx
andInWheel
.back_tick
function.pytest_tools.py
,scriptrunner.py
withpytest-console-scripts
The text was updated successfully, but these errors were encountered: