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

Fix * imports #138

Merged
merged 23 commits into from
Aug 19, 2024
Merged

Fix * imports #138

merged 23 commits into from
Aug 19, 2024

Conversation

stefsmeets
Copy link
Contributor

@stefsmeets stefsmeets commented Aug 12, 2024

This PR fixes all the from x import * imports. Avoiding * imports is generally considered a good practice. I used ruff to help with the formatting, partly addressing #131 as well.

To find import issues, you can use:

ruff check start_proteus.py  # single file
ruff check src/proteus       # directory
ruff check .                 # everything

Prepend --fix to sort imports and remove unused imports:

ruff check . --fix

I also set up a small pre-commit config so you can automatically have it sort imports on every commit, e.g.:

pre-commit install

Closes #136
Addresses #131

Todo

  • Setup github action
  • Add documentation

@stefsmeets stefsmeets marked this pull request as ready for review August 12, 2024 09:55
@stefsmeets stefsmeets requested review from timlichtenberg and removed request for lsoucasse August 12, 2024 10:01
@stefsmeets
Copy link
Contributor Author

stefsmeets commented Aug 12, 2024

Hi @nichollsh @timlichtenberg This PR is ready, could you have a look and let me know what you think?

Could you test it with some of your workflows to make sure no imports were missed? I don't expect any issues, except if there were implicit transitive dependencies (e.g. A imports * from B, B imports X from C, where A depends on X).

@stefsmeets stefsmeets mentioned this pull request Aug 12, 2024
4 tasks
@lsoucasse lsoucasse self-requested a review August 19, 2024 09:10
Copy link
Member

@lsoucasse lsoucasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stefsmeets, I spotted some missing imports. The default config script runs fine for me.

@stefsmeets
Copy link
Contributor Author

Hi @lsoucasse thanks for pointing those out, I fixed the issues.

@stefsmeets stefsmeets merged commit bc6a55d into master Aug 19, 2024
2 checks passed
@stefsmeets stefsmeets deleted the imports branch August 19, 2024 13:44
@stefsmeets stefsmeets restored the imports branch August 19, 2024 13:44
@stefsmeets stefsmeets deleted the imports branch August 19, 2024 13:44
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.

External module imports
2 participants