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

Improve the way to import optional modules #2809

Merged
merged 10 commits into from Dec 4, 2023
Merged

Improve the way to import optional modules #2809

merged 10 commits into from Dec 4, 2023

Conversation

seisman
Copy link
Member

@seisman seisman commented Nov 14, 2023

For optional modules, currently we do like this:

try:
    import contextily
except ImportError:
    contextily = None

but Mypy doesn't like it:

Incompatible types in assignment (expression has type "None", variable has type Module)

In this PR, we use another way to import optional modules:

try:
    import contextily

    _HAS_CONTEXTILY = True
except ImportError:
    _HAS_CONTEXTILY = False

See https://adamj.eu/tech/2021/12/29/python-type-hints-optional-imports/ for more detailed explanations.

Sometimes, modules like IPython are not directly used, then ruff reports F401 and recommends using importlib.util.find_spec.

Changes in this PR:

  • 5deebf8: Use a bool variable to indicate whether an optional module is available
  • c98c103: Use upper-case for constant name (Fix C0103)
  • b1c40b0: Use importlib.util.find_spec for modules that are not used (ruff F401)

@seisman seisman changed the title optional imports Improve the way to import optional modules Nov 14, 2023
@seisman seisman added the maintenance Boring but important stuff for the core devs label Nov 14, 2023
@seisman seisman added this to the 0.11.0 milestone Nov 14, 2023
@seisman seisman added the needs review This PR has higher priority and needs review. label Nov 14, 2023
Comment on lines 9 to 12
try:
import IPython
except ImportError:
IPython = None # pylint: disable=invalid-name
Copy link
Member

@weiji14 weiji14 Nov 14, 2023

Choose a reason for hiding this comment

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

Another option looking at https://stackoverflow.com/questions/61384752/how-to-type-hint-with-an-optional-import/77341843#77341843 seems to be something like:

IPython: Any = None
try:
    import IPython
except ImportError:
    pass

Though not sure if this is recommended.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@weiji14 weiji14 Nov 14, 2023

Choose a reason for hiding this comment

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

Looking at mypy (e.g. python/mypy#1297 (comment)), they also seem to recommend a try-except-else pattern? Something like:

try:
    import IPython as _IPython
except ImportError:
    IPython = None
else:
    IPython = _IPython

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's officially recommended and it's also more difficult to understand than IPython = None and _HAS_IPYTHON = False.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's go with _HAS_IPython unless there's a better recommended way 🙂

That said, we should probably decide first if adding mypy is something we want (#2808), otherwise the changes here aren't really needed.

Copy link
Member Author

@seisman seisman Nov 16, 2023

Choose a reason for hiding this comment

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

Since we're going to add type hints (#2794, #2812), we definitely need a static type checker. There are many static type checkers, like mypy, pyright, and pytype, in which mypy is the official one, so it's a good option to start.

pygmt/src/tilemap.py Outdated Show resolved Hide resolved
@seisman seisman mentioned this pull request Nov 27, 2023
7 tasks
@weiji14 weiji14 added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Dec 4, 2023
@seisman seisman merged commit 2dc3e6b into main Dec 4, 2023
16 checks passed
@seisman seisman deleted the optional-imports branch December 4, 2023 06:08
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants