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 Console and Unicode unit formatters #16465

Merged
merged 2 commits into from
May 22, 2024
Merged

Add type annotations to Console and Unicode unit formatters #16465

merged 2 commits into from
May 22, 2024

Conversation

eerovaher
Copy link
Member

Description

This continues the work of adding type annotations to units/format/ by annotating the Console formatter and the Unicode formatter, which is a subclass of Console.

  • 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 v7.0.0 milestone May 16, 2024
@eerovaher eerovaher requested review from mhvk and nstarman May 16, 2024 15:28
@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.

nstarman
nstarman reviewed May 16, 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.

All the annotations look good. A few suggestions for more.

astropy/units/format/console.py Outdated
@@ -30,15 +40,15 @@ class Console(base.Base):
_space = " "
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth annotating these class variables as well.

Suggested change
_space = " "
_space: ClassVar[str] = " "

astropy/units/format/unicode_format.py
@@ -43,7 +52,7 @@ def _format_unit_power(cls, unit, power=1):
return name

@classmethod
def _format_superscript(cls, number):
def _format_superscript(cls, number: str) -> str:
mapping = str.maketrans(
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated (though perhaps this PR is as good a place as any), but this dict should be moved to outside this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pull requests that add type annotations should not include any code changes.

astropy/units/format/unicode_format.py Outdated
@@ -29,11 +38,11 @@ class Unicode(console.Console):
_line = "─"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_line = "─"
_line: ClassVar[str] = "─"

astropy/units/format/unicode_format.py Show resolved Hide resolved
astropy/units/format/console.py Show resolved Hide resolved
@eerovaher eerovaher force-pushed the annotate-unit-format-console branch 2 times, most recently from b81df21 to 8b13610 Compare May 16, 2024 17:01
mhvk
mhvk approved these changes May 17, 2024
View reviewed changes
Copy link
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.

One comment about class variables, but happy to just approve - this looks nice!

astropy/units/format/console.py
_times = "*"
_line = "-"
_space = " "
_times: ClassVar[str] = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

I recall seeing some discussion by disgruntled typers about also typing variables, where what they should be is obvious and hence the typing only adds visual noise, with no benefit for humans. I'm happy to go with the flow here, but perhaps worth thinking about.

Copy link
Member

Choose a reason for hiding this comment

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

I think these end up being quite useful because it's important to know when you're modifying a class-level variable. Lots of unexpected behavior can happen when changing a class variable and you think it's an instance variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that makes sense - and indeed these singleton classes are a bit odd.

astropy/units/format/console.py
scale: str,
numerator: str,
denominator: str,
fraction: Literal[True, "inline", "multiline"] = "multiline",
Copy link
Contributor

Choose a reason for hiding this comment

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

Had to take a double-take, but, yes, the True without a False is indeed correct...

@eerovaher
Copy link
Member Author

@pllim, can you merge or should I rebase to get the documentation build to succeed?

@pllim
Copy link
Member

pllim commented May 20, 2024

@eerovaher , let's rebase to be sure. Thanks!

@eerovaher eerovaher force-pushed the annotate-unit-format-console branch from 8b13610 to 0044bbc Compare May 20, 2024 13:50
@eerovaher
Copy link
Member Author

There shouldn't be anything holding this back anymore.

@pllim pllim merged commit 4fcc4ce into astropy:main May 22, 2024
27 checks passed
@pllim
Copy link
Member

pllim commented May 22, 2024

Thanks for your patience!

@eerovaher eerovaher deleted the annotate-unit-format-console branch May 22, 2024 21:03
@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

@mhvk mhvk mhvk approved these changes

@nstarman nstarman nstarman approved these changes

Assignees
No one assigned
Labels
no-changelog-entry-needed typing related to type annotations units
Projects
None yet
Milestone
v7.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eerovaher @pllim @mhvk @nstarman