Skip to content

Add typing module to IO#15916

Merged
nstarman merged 10 commits into
astropy:mainfrom
nstarman:io-typing
Jan 20, 2024
Merged

Add typing module to IO#15916
nstarman merged 10 commits into
astropy:mainfrom
nstarman:io-typing

Conversation

@nstarman
Copy link
Copy Markdown
Member

@nstarman nstarman commented Jan 20, 2024

Describes PathLike and two flavors of file-like

  • 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.

@nstarman
Add typing module to IO
f6a6ab9 
Describes PathLike and two flavors of file-like

Signed-off-by: nstarman <[email protected]>
@nstarman nstarman added this to the v6.1.0 milestone Jan 20, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 20, 2024

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?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • 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.

@nstarman
changelog
ad75576 
Signed-off-by: nstarman <[email protected]>
@nstarman nstarman requested review from mhvk and taldcroft January 20, 2024 19:05
@astropy astropy deleted a comment from github-actions Bot Jan 20, 2024
@nstarman
docstring
83b9e77 
Signed-off-by: nstarman <[email protected]>
@nstarman
better privacy for non-public types
691e606 
Signed-off-by: nstarman <[email protected]>
@nstarman
py39 compat
bf42b6d 
Signed-off-by: nstarman <[email protected]>
nstarman
nstarman commented Jan 20, 2024
View reviewed changes
Comment thread astropy/io/typing.py Outdated
@nstarman nstarman marked this pull request as ready for review January 20, 2024 19:19
@nstarman nstarman mentioned this pull request Jan 20, 2024
Merged
1 task
@nstarman
documentation for RTD
c3f08bc 
Signed-off-by: nstarman <[email protected]>
@nstarman
fixes
ef9d050 
Signed-off-by: nstarman <[email protected]>
@nstarman
there’s no typing module
5fbf68d 
Signed-off-by: nstarman <[email protected]>
@nstarman
ibid
e0529a8 
Signed-off-by: nstarman <[email protected]>
mhvk
mhvk approved these changes Jan 20, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

I cannot really judge correctness here, but it certainly looks good. Would it make sense to annotate some actual pieces of code? E.g., fits.open (hdu.hudlist.fitsopen)? But also fine to do things in follow-up (feeling free to still change this; nice to get it in well before 6.1 so there is time to iterate if needed).

Comment thread astropy/io/typing.py
if TYPE_CHECKING:
from typing import TypeAlias

_T_co = TypeVar("_T_co", covariant=True)
Copy link
Copy Markdown
Contributor

@mhvk mhvk Jan 20, 2024

Choose a reason for hiding this comment

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

This part I really have no idea what is being done...

@nstarman
Copy link
Copy Markdown
Member Author

nstarman commented Jan 20, 2024

Would it make sense to annotate some actual pieces of code? E.g., fits.open (hdu.hudlist.fitsopen)?

See #15917!

@nstarman nstarman merged commit 8245f4e into astropy:main Jan 20, 2024
@nstarman nstarman deleted the io-typing branch January 20, 2024 21:43
eerovaher
eerovaher reviewed Jan 22, 2024
View reviewed changes
Comment thread astropy/io/typing.py
_T_co = TypeVar("_T_co", covariant=True)
_T_contra = TypeVar("_T_contra", contravariant=True)

PathLike: TypeAlias = Union[str, bytes, os.PathLike]
Copy link
Copy Markdown
Member

@eerovaher eerovaher Jan 22, 2024

Choose a reason for hiding this comment

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

Is it a good idea to have a type called PathLike given that os.PathLike exists in the standard library?

Copy link
Copy Markdown
Member Author

@nstarman nstarman Jan 22, 2024

Choose a reason for hiding this comment

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

I based the hint off of https://docs.python.org/3/glossary.html#term-path-like-object

Copy link
Copy Markdown
Member Author

@nstarman nstarman Jan 22, 2024

Choose a reason for hiding this comment

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

It's quite annoying that Python made os.PathLike given that their own definition of path-like includes str and bytes.

Copy link
Copy Markdown
Member

@eerovaher eerovaher Jan 22, 2024

Choose a reason for hiding this comment

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

We can reconsider this if it ever becomes a problem.

Comment thread docs/io/typing.rst
Copy link
Copy Markdown
Member

@eerovaher eerovaher Jan 22, 2024

Choose a reason for hiding this comment

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

We have just started adding type annotations to astropy, so we might want to make big changes to them. If we are going to have public documentation about typing then there should be a warning somewhere that makes it clear that typing support in astropy is experimental and subject to change without notice.

@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

@eerovaher eerovaher eerovaher left review comments

@mhvk mhvk mhvk approved these changes

@taldcroft taldcroft Awaiting requested review from taldcroft

Assignees

No one assigned

Labels

Docs io.registry typing related to type annotations

Projects

Astropy Infrastructure
Status: Done

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nstarman @mhvk @eerovaher