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.

Sign up for GitHub

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

Jump to bottom

Add type annotations to introspection functions #16234

Merged
merged 1 commit into from
Apr 3, 2024
Merged

Add type annotations to introspection functions #16234

merged 1 commit into from
Apr 3, 2024

Conversation

eerovaher
Copy link
Member

Description

All the functions defined in astropy/utils/introspection.py are now either annotated or deprecated.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@eerovaher eerovaher added this to the v6.1.0 milestone Mar 25, 2024
@eerovaher eerovaher requested a review from pllim March 25, 2024 15:26
@github-actions GitHub Actions
Copy link

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

eerovaher
eerovaher commented Mar 25, 2024
View reviewed changes
astropy/utils/introspection.py

from packaging.version import Version

from .decorators import deprecated

if TYPE_CHECKING:
from types import FrameType, ModuleType
Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation build failed because ModuleType couldn't be found. Is there a way to set TYPE_CHECKING to True when building the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Weird that it fails even with from __future__ import annotations. Maybe quoting it would work ? (but you may also need noqa comments to prevent ruff from undoing it)

Copy link
Member Author

Choose a reason for hiding this comment

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

RTD is complaining about ModuleType, but not about FrameType or Literal. That's making me think that it's the fact I've used ModuleType in a return annotation that's causing problems somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not comfortable enough around RTD to say anything with confidence, but this smells like a potential bug on their side ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's part of a broader Sphinx issue sphinx-doc/sphinx#11991, a quick hacky way is to just add py:class ModuleType to docs/nitpick-exceptions.

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 tried a few different ways of importing ModuleType, but I couldn't get the documentation build to work properly. It seems that adding a new nitpick exception is the optimal solution under the circumstances.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, RTD support is suboptimal

neutrinoceros
neutrinoceros reviewed Mar 25, 2024
View reviewed changes
astropy/utils/introspection.py
@@ -86,7 +93,7 @@ def resolve_name(name, *additional_parts):
return ret


def minversion(module, version, inclusive=True):
def minversion(module: ModuleType | str, version: str, inclusive: bool = True) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks a lot for doing this, I can never remember the return type of this function, I'm thrilled that soon my text editor will tell me !

astropy/utils/introspection.py
@@ -146,7 +153,9 @@ def minversion(module, version, inclusive=True):
return Version(module_version) > Version(version)


def find_current_module(depth=1, finddiff=False):
def find_current_module(
depth: int = 1, finddiff: bool | list[Literal[True] | str | ModuleType] = False
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest splitting this line to make it clearer at a glance that there are just 2 arguments here

Suggested change
depth: int = 1, finddiff: bool | list[Literal[True] | str | ModuleType] = False
depth: int = 1,
finddiff: bool | list[Literal[True] | str | ModuleType] = False,

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 have a weak preference for not splitting lines if we don't have to, but if someone else says they also prefer writing this out on two lines then I will make the change.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's fine as is.

@eerovaher
Add type annotations to introspection functions
de682e6 
A Sphinx bug requires the documentation configuration to be updated too.
@eerovaher eerovaher requested a review from eteq March 26, 2024 15:38
pllim
pllim reviewed Mar 29, 2024
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

I haven't been much into typing so not sure if I am best person to review.

@eerovaher eerovaher requested a review from nstarman April 2, 2024 15:04
nstarman
nstarman approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

Everything looks good. But it's strange that codecov is saying all the line changes aren't covered by CI!

@pllim pllim merged commit 4a89cfd into astropy:main Apr 3, 2024
26 of 27 checks passed
@pllim
Copy link
Member

pllim commented Apr 3, 2024

Thanks, all!

@eerovaher eerovaher deleted the annotate-introspection branch April 3, 2024 15:07
@eerovaher eerovaher added the typing related to type annotations label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pllim pllim pllim left review comments

@MridulS MridulS MridulS left review comments

@neutrinoceros neutrinoceros neutrinoceros left review comments

@nstarman nstarman nstarman approved these changes

@eteq eteq Awaiting requested review from eteq

Assignees
No one assigned
Labels
no-changelog-entry-needed typing related to type annotations utils
Projects
None yet
Milestone
v6.1.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@eerovaher @pllim @MridulS @nstarman @neutrinoceros