Skip to content

Meta folder - #15166

Merged
mhvk merged 3 commits into
astropy:mainfrom
nstarman:meta-folder
Sep 8, 2023
Merged

Meta folder#15166
mhvk merged 3 commits into
astropy:mainfrom
nstarman:meta-folder

Conversation

@nstarman

@nstarman nstarman commented Aug 13, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

There's a lot of different ideas happening in astropy/utils/metadata.py: new exceptions and warnings, metadata merging mechanics, a metadata attribute, and attributes stored in metadata. This PR separates these concerns into different files, collecting them in a new module -- astropy/utils/metadata. There is no change in public API.
There is change in semi-public API.

Plz squash & merge

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

github-actions Bot commented Aug 13, 2023 •
edited by mhvk
Loading

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 13, 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 marked this pull request as ready for review August 13, 2023 19:48
@nstarman
nstarman requested a review from taldcroft as a code owner August 13, 2023 19:48
@mhvk mhvk mentioned this pull request Aug 14, 2023
Open
mhvk
mhvk requested changes Aug 14, 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.

It may indeed be time to make a new submodule, but I think a renaming exercise should really not do anything other than rename, i.e., not make MetaAttribute a dataclass, nor add any typing information.

It would also be good to have a more general discussion about whether we want to indicate things below the top level are private by prepending _ to file names. I know other projects are doing it, but we have a pretty clear system in place and with changes like here and in cosmology we're going to be in a mixed and hence unclear state for quite a while (a state where the sub-packages that do not rename will be the ones with increased risk of user mistakes). I feel quite strongly this change should not be made piece-wise so raised a separate issue about this -- see #15169. I think it would be best not to merge this as is until there has been some decision.

@mhvk mhvk mentioned this pull request Aug 14, 2023
Closed
@nstarman

nstarman commented Aug 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

I'm fine to break up this PR into 2: one for re-org and another for improvements. Re the file names, any suggestion?

@mhvk

mhvk commented Aug 14, 2023

Copy link
Copy Markdown
Contributor

Yes, I think it should be broken up; for the filenames, I think we should follow precedent and not use underscores. They can be added later if APE 22 is accepted.

@nstarman

nstarman commented Aug 15, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

SGTM.

@mhvk, done.

@nstarman
nstarman requested a review from mhvk August 18, 2023 03:13
@nstarman

nstarman commented Aug 31, 2023

Copy link
Copy Markdown
Member Author

Ping @mhvk. I think this should be merged after #15237

@nstarman

nstarman commented Sep 2, 2023

Copy link
Copy Markdown
Member Author

Ok. #15237 is merged. This is ready!

@pllim

pllim commented Sep 5, 2023

Copy link
Copy Markdown
Member

If there is some change, then need a change log. Thanks!

@nstarman

nstarman commented Sep 6, 2023

Copy link
Copy Markdown
Member Author

Changelog added!

@nstarman

nstarman commented Sep 6, 2023

Copy link
Copy Markdown
Member Author

Ping @mhvk. Hopefully an easy review now!

mhvk
mhvk reviewed Sep 7, 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.

We could avoid changes to imports from test_meta (and thus the need for possible similar changes outside astropy) if we kept the test files in the same location. Unlike for astropy.coordinates.angles, I don't have a strong opinion on what is better. utils.iers and utils.masked have their tests inside and this is logical; that suggests to do what you have done for utils.metadata. On the other hand, for ies and (to a less extend) for masked there is a case that they don't belong in utils but should be on their own, while metadata is exactly in the right place. And clearly there are imports from the tests... Then again, utils.xml does have its own tests (though that has C code). Overall, what you have is probably best, but thought I would bring it up anyway.

Otherwise, my only remaining comment is about __all__ in lower-level files. Let's stick to current precedent for those, for now at least, i.e., fill them with those items that should be imported in __init__.py.

Comment thread astropy/utils/metadata/core.py Resolved
Comment thread astropy/utils/metadata/__init__.py Resolved
Comment thread docs/changes/utils/15166.api.rst Resolved
@nstarman
fix changelog
a2535ad 
Signed-off-by: nstarman <[email protected]>
@nstarman
nstarman requested a review from mhvk September 7, 2023 19:22
mhvk
mhvk approved these changes Sep 8, 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 all OK to me now! Thanks, @nstarman

@mhvk
mhvk merged commit 04e9a16 into astropy:main Sep 8, 2023
@nstarman
nstarman deleted the meta-folder branch September 8, 2023 12:05
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
@taldcroft taldcroft Awaiting requested review from taldcroft taldcroft is a code owner

Assignees

No one assigned

Labels

Refactoring utils

Projects

None yet

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nstarman @mhvk @pllim