Skip to content

MetaData on dataclasses and default is None - #15237

Merged
nstarman merged 4 commits into
astropy:mainfrom
nstarman:metadata-get
Aug 31, 2023
Merged

MetaData on dataclasses and default is None#15237
nstarman merged 4 commits into
astropy:mainfrom
nstarman:metadata-get

Conversation

@nstarman

@nstarman nstarman commented Aug 28, 2023

Copy link
Copy Markdown
Member

Partially replaces #15167

@nstarman nstarman added this to the v6.0 milestone Aug 28, 2023
@github-actions github-actions Bot added the utils label Aug 28, 2023
@github-actions

github-actions Bot commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

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.

@github-actions

github-actions Bot commented Aug 28, 2023

Copy link
Copy Markdown
Contributor

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@nstarman

nstarman commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

changelog worthy?

@nstarman
nstarman requested a review from mhvk August 28, 2023 16:32
@pllim

pllim commented Aug 28, 2023

Copy link
Copy Markdown
Member

changelog worthy?

Not sure. If you only want to use this internally in this repo, then no. If you want enduser to use this as a standalone feature, then yes, but I thought utils is mainly for internal use?

@nstarman

nstarman commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

Hmm. Better safe than sorry, I guess. 🤷

mhvk
mhvk reviewed Aug 28, 2023
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, looks good! Comments inline. I do think we should add a changelog entry, but maybe let that spell out that this class is mostly used internally,

The ``astropy.utils.metadata.MetaData`` class, which is used throughout astropy
to carry metadata on tables, columns, etc., can now also be used on dataclasses.

FWIW, I'd count this as a bug fix (but not one that needs to be backported, since we don't use it yet).

Comment thread astropy/utils/metadata.py Resolved
Comment thread astropy/utils/metadata.py Resolved
Comment thread astropy/utils/metadata.py Resolved
@nstarman nstarman changed the title MetaData works on dataclasses MetaData on dataclasses and default is None Aug 30, 2023
@nstarman
nstarman requested a review from mhvk August 30, 2023 16:28
@nstarman
nstarman marked this pull request as ready for review August 30, 2023 16:28
@nstarman
nstarman requested a review from a team as a code owner August 30, 2023 16:28
mhvk
mhvk reviewed Aug 31, 2023
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This direction really seems fine to me. Just needs cleanup! I would comment where we now return None that we do this for dataclasses.

@nstarman
nstarman requested a review from mhvk August 31, 2023 16:09
@nstarman

nstarman commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

In a followup PR I'm going to add the default_factory arg that will allow someone to change the default from an empty OrderedDict to any type of Mapping. We'll keep the default as OrderedDict for backwards compatibility.

@nstarman nstarman mentioned this pull request Aug 31, 2023
Merged
mhvk
mhvk approved these changes Aug 31, 2023
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. I think a comment about returning None would still be useful, but it is by no means essential, so I'll approve now and let you decide.

Comment thread astropy/utils/metadata.py Resolved
@nstarman
comment
9f4988a 
Signed-off-by: nstarman <[email protected]>
@nstarman
nstarman enabled auto-merge (squash) August 31, 2023 17:38
@nstarman

nstarman commented Aug 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Thanks @mhvk for the reviews and suggestions! It should auto-squash merge when CI passes.

@nstarman
nstarman merged commit b3a4aa2 into astropy:main Aug 31, 2023
@nstarman nstarman mentioned this pull request Sep 1, 2023
Merged
1 task
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

Assignees

No one assigned

Labels

utils

Projects

Cosmology, the Expansion
Status: Done

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nstarman @pllim @mhvk