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

Code cleanup suggestions. #125

Open
6 of 8 tasks
HexDecimal opened this issue Sep 21, 2021 · 2 comments
Open
6 of 8 tasks

Code cleanup suggestions. #125

HexDecimal opened this issue Sep 21, 2021 · 2 comments

Comments

@HexDecimal
Copy link
Collaborator

HexDecimal commented Sep 21, 2021

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 the 0.9.1 release. A CONTRIBUTING 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__ and back_tick. I think InWheelCtx needs to be refactored or removed (or finish #46 and fix things there,) and all calls to back_tick should be replaced with subprocess.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 replace scriptrunner.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.

  • Enable auto formatters Black and isort.
  • Set up CONTRIBUTING file.
  • Fix type issues with InWheelCtx and InWheel.
  • Fix back_tick function.
  • Clean up pytest_tools.py,
  • Replace scriptrunner.py with pytest-console-scripts
  • Add Issue templates.
  • Add PR templates.
@matthew-brett
Copy link
Owner

All good suggestions. Would you like me to review #93? I suppose we should keep / deprecate backticks for a bit, just in case, but yes, all uses should be replaced with subprocess.run.

@HexDecimal
Copy link
Collaborator Author

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.
HexDecimal added a commit that referenced this issue Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants