Skip to content

feat(coords): deprecate .get_name() for .name - #17503

Merged
mhvk merged 6 commits into
astropy:mainfrom
nstarman:coord-rep-get_name
Dec 5, 2024
Merged

feat(coords): deprecate .get_name() for .name#17503
mhvk merged 6 commits into
astropy:mainfrom
nstarman:coord-rep-get_name

Conversation

@nstarman

@nstarman nstarman commented Dec 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Using classproperty we can simplify .get_name() to just .name.
I'll add changelog, etc if you LMK what you think about the change in general.

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@nstarman nstarman added this to the v7.1.0 milestone Dec 4, 2024
@github-actions

github-actions Bot commented Dec 4, 2024

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?
  • 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 Dec 4, 2024

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 requested review from eerovaher and mhvk December 4, 2024 22:54
mhvk
mhvk reviewed Dec 5, 2024
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.

I think this is better than .get_name(), but wonder whether it isn't more logical to just set the name (following the same string editing) in __init_subclass__ - as is done for frames. That has the advantage that someone can define the name on the class itself (which is what is required for time format registration).

Comment thread astropy/coordinates/representation/base.py Resolved
@nstarman

nstarman commented Dec 5, 2024

Copy link
Copy Markdown
Member Author

__init_subclass__ SGTM

@nstarman
nstarman requested a review from mhvk December 5, 2024 15:50
@nstarman
📝: add changelog
3ccc5c9 
Signed-off-by: nstarman <[email protected]>
@nstarman

nstarman commented Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mhvk my only concern with __init_subclass__ is that .name is now mutable.
What about setting ._name and having a classproperty(name) ?

@mhvk

mhvk commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

@mhvk my only concern with __init_subclass__ is that .name is now mutable. What about setting ._name and having a classproperty(name) ?

This is true for coordinate frames and time formats too. I don't think we have to try to protect against changing attributes.

@nstarman
nstarman marked this pull request as ready for review December 5, 2024 16:31
mhvk
mhvk reviewed Dec 5, 2024
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.

I like this better! Some more detailed comments in-line. Also, do check the API policy for the milestone (think deprecation in a minor version is OK, but not sure).

Comment thread astropy/coordinates/representation/base.py Resolved
Comment thread astropy/coordinates/representation/base.py Resolved
Comment thread astropy/coordinates/representation/base.py Resolved
Comment thread astropy/coordinates/representation/base.py Resolved
Comment thread astropy/coordinates/representation/base.py Resolved
Comment thread astropy/coordinates/representation/base.py Resolved
mhvk
mhvk reviewed Dec 5, 2024
View reviewed changes
Comment thread astropy/coordinates/representation/base.py Resolved
@nstarman
💄: minor adjustments
f6ad062 
Signed-off-by: nstarman <[email protected]>
@nstarman
nstarman requested a review from mhvk December 5, 2024 17:19
mhvk
mhvk reviewed Dec 5, 2024
View reviewed changes
Comment thread astropy/coordinates/representation/base.py Outdated
)
if not hasattr(cls, "name"):
cls.name = default_name
elif BaseRepresentationOrDifferential not in cls.__bases__ and cls.name in (

@mhvk mhvk Dec 5, 2024

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.

Looking at this again, I'm actually not sure why this first part is in here - just because it doesn't have .name itself? But then wouldn't that be caught? I.e., why not just have,

# Set default name if no name was given or we inherited it
if (name := getattr(cls, "name", None)) is None or name in (getattr(base, "name", None) for base in cls.__bases__):
   cls.name = cls.__name__.lower()...

Actually, can we not just do if "name" not in cls.__dict__?

@nstarman nstarman Dec 5, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we can.

@nstarman nstarman Dec 5, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

mhvk
mhvk approved these changes Dec 5, 2024
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.

OK, looks all good now. I looked at https://github.com/astropy/astropy-APEs/blob/main/APE21.rst and it seems clear this can go in 7.1.

@mhvk mhvk added API change PRs and issues that change an existing API, possibly requiring a deprecation period Refactoring labels Dec 5, 2024
@mhvk
mhvk merged commit 950f9e0 into astropy:main Dec 5, 2024
@nstarman
nstarman deleted the coord-rep-get_name branch December 5, 2024 21:52
@mhvk mhvk mentioned this pull request Jan 3, 2025
Merged
1 task done
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
@eerovaher eerovaher Awaiting requested review from eerovaher

Assignees

No one assigned

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period coordinates Refactoring

Projects

None yet

Milestone

v7.1.0

Development

Successfully merging this pull request may close these issues.

2 participants

@nstarman @mhvk