Skip to content

Remove metaclasses from astropy.coordinates - #11090

Merged
mhvk merged 8 commits into
astropy:masterfrom
adrn:coordinates/excise-metaclasses
Nov 26, 2020
Merged

mhvk merged 8 commits into
astropy:masterfrom
adrn:coordinates/excise-metaclasses

Conversation

@adrn

@adrn adrn commented Nov 25, 2020

Copy link
Copy Markdown
Member

This PR partially addresses #7144 (specifically for classes in astropy.coordinates).

I moved the logic from various metaclasses (FrameMeta, MetaBaseRepresentation, MetaBaseDifferential) into the relevant __init_subclass__ methods.

Note that I wasn't able to remove FrameMeta entirely, because BaseCoordinateFrame still requires OrderedDescriptorContainer as a metaclass. But when I tried to add metaclass=OrderedDescriptorContainer directly, I get a metaclass conflict error.

@adrn adrn added this to the v4.3 milestone Nov 25, 2020
@adrn
adrn requested review from eteq and mhvk November 25, 2020 20:29

@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.

@adrn - this looks good (and is good to do). Apart from the request to add **kwargs, the rest are just suggestions.

Comment thread astropy/coordinates/baseframe.py Outdated
Comment thread astropy/coordinates/baseframe.py
Comment thread astropy/coordinates/baseframe.py Outdated
Comment thread astropy/coordinates/baseframe.py Outdated
Comment thread astropy/coordinates/representation.py Outdated
@adrn
adrn force-pushed the coordinates/excise-metaclasses branch from 226f5a0 to 3d1fe42 Compare November 26, 2020 15:33
@adrn

adrn commented Nov 26, 2020

Copy link
Copy Markdown
Member Author

Hm, where should I put the changelog entry for this?

@mhvk

mhvk commented Nov 26, 2020

Copy link
Copy Markdown
Contributor

Maybe "other changes"? In principle, it is just an internal refactoring with no user-facing consequences, so not sure if it is needed...

@adrn

adrn commented Nov 26, 2020

Copy link
Copy Markdown
Member Author

@mhvk Ah right, I don't think I ever noticed that section 😅

@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 good to me! I'll just merge since this does not really change anything, just make it easier to follow.

@embray

embray commented Nov 27, 2020

Copy link
Copy Markdown
Member

@adrn I took care of OrderedDescriptorContainer in #11099.

embray added a commit to embray/astropy that referenced this pull request Nov 27, 2020
mhvk added a commit that referenced this pull request Nov 29, 2020
embray added a commit to embray/astropy that referenced this pull request Dec 31, 2020
embray added a commit to embray/astropy that referenced this pull request Jan 16, 2021
namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021
namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021
nstarman pushed a commit to nstarman/astropy that referenced this pull request Feb 28, 2021
Akshat1Nar pushed a commit to Akshat1Nar/astropy that referenced this pull request Mar 9, 2021
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.

4 participants