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

DM-34331: Use black/isort #56

Merged
merged 11 commits into from Apr 7, 2022
Merged

DM-34331: Use black/isort #56

merged 11 commits into from Apr 7, 2022

Conversation

timj
Copy link
Member

@timj timj commented Apr 6, 2022

No description provided.

@timj timj force-pushed the tickets/DM-34331 branch 3 times, most recently from bc87943 to ef9952e Compare April 7, 2022 18:06
@timj timj force-pushed the tickets/DM-34331 branch 2 times, most recently from 491a782 to b334c69 Compare April 7, 2022 19:39
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@f4b76ee). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #56   +/-   ##
=======================================
  Coverage        ?   77.79%           
=======================================
  Files           ?       36           
  Lines           ?     2968           
  Branches        ?      471           
=======================================
  Hits            ?     2309           
  Misses          ?      542           
  Partials        ?      117           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b76ee...ce31c5a. Read the comment docs.

Copy link
Contributor

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

I wonder if you can add type hints for dynamic properties by setting the __annotations__ class variable when you set up the properties?
That might also allow you to do something with the extension properties. Does mypy complain when running test_extensions.py?

altaz_begin: astropy.coordinates.AltAz
science_program: str
observation_type: str
observation_id: 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 wonder if you can do this by adding the types to ObservationInfo.__annotations__ when the properties are created?

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'm fairly certain that mypy does not support __annotations__ because it does not actually run the python code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@TallJimbo do you have any thoughts on this question as to whether it's possibly for mypy to derive properties?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe there's some way to do it via a MyPy plugin, but I'm pretty sure there's nothing you can do short of that that isn't equivalent to what you've done here.

to_altaz_begin: Callable[[MetadataTranslator], astropy.coordinates.AltAz]
to_science_program: Callable[[MetadataTranslator], str]
to_observation_type: Callable[[MetadataTranslator], str]
to_observation_id: Callable[[MetadataTranslator], str]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

timj added 5 commits April 7, 2022 14:06
mypy needs them because this helper class does not inherit
from unittest.TestCase.
MetadataTranslator and ObservationInfo have dynamic properties
but that works against mypy. Declare the standard set so that
mypy does not complain about its own code.
@timj timj merged commit 1ca0471 into main Apr 7, 2022
@timj timj deleted the tickets/DM-34331 branch April 7, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants