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

deprecate ignore_implicit_conversion and implicit_conversion #1724

Merged
merged 5 commits into from
May 13, 2024

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Jan 3, 2024

Description

This is work towards fixing #1723

This PR deprecates "implicit conversion" and the flag controlling warning messages when this behavior occurs (ignore_implicit_conversion). See the above issue for more details. In brief, this prevents extensions from adding support for namedtuple classes.

The removal of this feature (after the deprecation period, which this PR will start) will mean that attempting to add a namedtuple to the asdf tree (without implementing a converter for the namedtuple) will result in an error. The effectively drops the "support" we currently have for namedtuple (which does not roundtrip, loses all namedtuple specifics and saves these instances as lists). User code that wishes to continue to support namedtuple instances will need to:

  • convert them all to lists prior to saving
  • implement a converter for their namedtuple classes to allow roundtripping

Checklist:

  • pre-commit checks ran successfully
  • tests ran successfully
  • for a public change, a changelog entry was added
  • for a public change, documentation was updated
  • for any new features, unit tests were added

@braingram braingram marked this pull request as ready for review January 3, 2024 21:41
@braingram braingram requested a review from a team as a code owner January 3, 2024 21:41
@braingram braingram force-pushed the named_tuple branch 2 times, most recently from d9bd0c8 to 7db96b0 Compare February 27, 2024 14:57
@braingram braingram force-pushed the named_tuple branch 2 times, most recently from a1da7a8 to a0371cc Compare May 10, 2024 15:55
@braingram
Copy link
Contributor Author

braingram commented May 10, 2024

Runs of romancal and jwst regtestsshow noAsdfDeprecationWarnings indicating so neither of these packages serialize anamed_tuple.

Romancal regtest show no AsdfDeprecationWarning messages with this PR: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FRoman-Developers-Pull-Requests/detail/Roman-Developers-Pull-Requests/753/pipeline/247

JWST regtests show 2 unreleated errors: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/1435/tests
and no AsdfDeprecationWarning messages

Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram braingram merged commit 019c6a4 into asdf-format:main May 13, 2024
48 checks passed
@braingram braingram deleted the named_tuple branch May 13, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants