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

Make @particle_input compatible with from __future__ import annotations #2479

Merged

Conversation

namurphy
Copy link
Member

@namurphy namurphy commented Jan 26, 2024

Description

@JaydenR2305 found a bug that @particle_input did not work on an instance method when preceded by from __future__ import annotations, which delays the evaluation of annotations. The purpose of this PR is to fix that bug.

However, at the moment I don't actually know how to fix it, so I'm going to start with writing some purposefully failing tests that reproduce the bug, and then try to get it to work somehow.

Wish me luck! 🙃

Related issues

Bug originally observed in #2245.

@github-actions github-actions bot added plasmapy.particles Related to the plasmapy.particles subpackage testing labels Jan 26, 2024
@namurphy namurphy added the bugfix PRs that fix a reported bug label Jan 26, 2024
@github-actions github-actions bot added the bug Issues describing unexpected behavior or defects. Remember: a bug is a sign of a missing test! label Jan 27, 2024
Copy link

codecov bot commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c6497f2) 97.37% compared to head (9c8f429) 97.37%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2479      +/-   ##
==========================================
- Coverage   97.37%   97.37%   -0.01%     
==========================================
  Files         104      104              
  Lines        9401     9399       -2     
==========================================
- Hits         9154     9152       -2     
  Misses        247      247              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -9,7 +9,7 @@
from collections.abc import Callable, Iterable, MutableMapping
from inspect import BoundArguments
from numbers import Integral, Real
from typing import Any, Optional, TypedDict, Union
from typing import Any, Optional, TypedDict, Union, get_type_hints
Copy link
Member Author

@namurphy namurphy Feb 1, 2024

Choose a reason for hiding this comment

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

ChatGPT suggested using typing.get_type_hints because other methods returned the string representations of the annotations rather than the actual objects. I don't know how long this would have taken me to figure out otherwise!

Copy link
Contributor

Choose a reason for hiding this comment

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

ChatGPT 😱

@namurphy
Copy link
Member Author

namurphy commented Feb 1, 2024

This PR should make @particle_input compatible for when postponed evaluation of annotations eventually(?) becomes default (PEP 563, PEP 649). Probably. Hopefully!

@namurphy namurphy marked this pull request as ready for review February 1, 2024 05:56
@namurphy namurphy requested a review from a team as a code owner February 1, 2024 05:56
@namurphy namurphy requested review from ejohnson-96 and removed request for a team February 1, 2024 05:56
Copy link
Contributor

@ejohnson-96 ejohnson-96 left a comment

Choose a reason for hiding this comment

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

What an annoying little bug 👍

@namurphy
Copy link
Member Author

namurphy commented Feb 1, 2024

What an annoying little bug 👍

If postponed evaluation of annotations had become the default in a future version of Python before this was fixed, this would have been an annoying little bug that broke the entire package! 🙃

@namurphy
Copy link
Member Author

namurphy commented Feb 1, 2024

@JaydenR2305 — thank you for finding this bug in #2245! It should be fixed once this is merged.

@namurphy namurphy merged commit 38692b2 into PlasmaPy:main Feb 1, 2024
16 checks passed
@namurphy namurphy deleted the particle-input-from-future-annotations branch February 7, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues describing unexpected behavior or defects. Remember: a bug is a sign of a missing test! bugfix PRs that fix a reported bug plasmapy.particles Related to the plasmapy.particles subpackage testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants