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

STYLE: Prefer absolute imports #75

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Contributor

Prefer absoulute import statements: Python PEP 8 recomends using absolute imports:
https://peps.python.org/pep-0008/#imports

"Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages) (...)."

Prefer absoulute import statements: Python PEP 8 recomends using
absolute imports:
https://peps.python.org/pep-0008/#imports

"Absolute imports are recommended, as they are usually more readable and
tend to be better behaved (or at least give better error messages)
(...)."
@jhlegarreta
Copy link
Contributor Author

Keeping this as a draft until a tag/release is made. xref: #67 (comment)

@demianw
Copy link
Owner

demianw commented Dec 11, 2024

I disagree with this one. Most packages, for instance scikit learn or nilearn, for instance use relative imports.

@demianw
Copy link
Owner

demianw commented Dec 11, 2024

Keeping this as a draft until a tag/release is made. xref: #67 (comment)

Please don't keep them as drafts if you want me to review them, just keep them as draft if they are work in progress. Even if you think we should make a tag/release

@jhlegarreta
Copy link
Contributor Author

Nilearn recommends absolute imports even if they may not be consistent in their code base/may have switched preferences:
https://nilearn.github.io/stable/development.html#contribution-guidelines

scikit-learn recommends relative imports in their code base but absolute imports in unit tests:
https://scikit-learn.org/stable/developers/develop.html#coding-guidelines

I still believe absolute imports are helpful, but do not have other arguments at present.

@jhlegarreta
Copy link
Contributor Author

Please don't keep them as drafts if you want me to review them, just keep them as draft if they are work in progress. Even if you think we should make a tag/release

Was keeping them as drafts to avoid accidentally merging them before tagging. I know tagging can be done from any given commit, but thought it was cleaner this way.

@demianw
Copy link
Owner

demianw commented Dec 11, 2024

Let's keep this one for further discussion and continue moving forward with other PRs. Thanks for such a granular work !

If you look at sklearn's code, all imports are relative e. g. : https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/linear_model/__init__.py

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.

2 participants