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

Avoid * imports, explicitly declare exported symbols #62

Merged
merged 3 commits into from
Aug 29, 2019
Merged

Conversation

Anthchirp
Copy link
Member

less from x import *
more __all__=(...)

@Anthchirp Anthchirp self-assigned this Jul 3, 2019
@Anthchirp
Copy link
Member Author

This pull request is not meant to be merged straightaway, but as a basis to discuss possible import * and __all__ conventions.

  • The best way for automatic linting and thus the way that makes flake8 the happiest is to always avoid import * and to declare __all__. This means that if you want to export symbols that come from C++ you will have to declare them twice. This is annoying if you need to add or remove symbols.
  • On the other hand it does make your API very clear: This function I imported from C++ is supposed to be made available through here.
  • One can argue that files that only do this, and do not contain any actual Python code can be cleaner by having a import * and declare an __all__. This would be mostly acceptable to flake8 and avoid duplication in the code. One example is format/image.py in this pull request.
  • Whereas if your file also contains wrangling code (eg. imageset.py) it makes sense to not do import * to make it clear where symbols come from, and declare __all__ to make clear where symbols go to. For example the different *Aux classes do not need to be exported.

since this file *only* imports and exports symbols we can use 'import *'
here.

cf. dials/dials#838
@Anthchirp
Copy link
Member Author

unless someone shouts will (squash) merge this shortly

@Anthchirp
Copy link
Member Author

no complaints => merge

@Anthchirp Anthchirp merged commit c98a2a3 into master Aug 29, 2019
@Anthchirp Anthchirp deleted the nostar branch August 29, 2019 14:08
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