-
Notifications
You must be signed in to change notification settings - Fork 224
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
Convert relative imports to absolute imports #754
Conversation
pygmt/tests/test_meca.py
Outdated
from pygmt.helpers import GMTTempFile | ||
from ..helpers import GMTTempFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're on this. Do we want to actually want to continue using relative imports or switch to absolute imports? From https://realpython.com/absolute-vs-relative-python-imports/#pros-and-cons-of-absolute-imports:
Absolute imports are preferred because they are quite clear and straightforward. It is easy to tell exactly where the imported resource is, just by looking at the statement. Additionally, absolute imports remain valid even if the current location of the import statement changes. In fact, PEP 8 explicitly recommends absolute imports.
One clear advantage of relative imports is that they are quite succinct. Depending on the current location, they can turn the ridiculously long import statement you saw earlier to something as simple as this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. PyGMT doesn't have a deep folder structure, so it seems absolute imports are better. I'm OK with changing relative imports to absolute imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if you want me to convert relative imports to absolute imports in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I much prefer absolute imports actually. It'll also help a bit with #686. I think it's fine to do it here in this PR.
And just for context, I think the relative imports were from the old days of gmt-python. It's easier to change the package name with relative imports, but I don't think we'll change pygmt's name anymore now that it's more established.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know any tools which can do the conversion automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Probably just regular search and replace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before that, I think we should let pylint check and warn relative imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to add either W0403
or relative-import
to disable=
or enable=
in .pylintrc
, but none of them warn about relative imports. Am I misunderstanding how pylint works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we have no-absolute-import
disabled, does that do anything? Actually have no clue how to set this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not related:
no-absolute-import (W1618):
import missing
from __future__ import absolute_import
Used when an import is not accompanied by from future import absolute_import (default behaviour in Python 3)
This PR is almost done, but we have this error:
@weiji14 Do you have any idea what it means? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Description of proposed changes
This PR converts all relative imports to absolute imports.
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Notes
/format
in the first line of a comment to lint the code automatically