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

Fix confusing argument name in realize_frame #7923

Merged
merged 5 commits into from Oct 26, 2018

Conversation

adrn
Copy link
Member

@adrn adrn commented Oct 19, 2018

As a result of a large copy-pasta for v3.0 (or 2.0?), I renamed a bunch of things representation -> representation_type. Well, this one was changed by accident: realize_frame expects a representation with data, not a representation type. I've changed the name now to make this more clear...but this is kind of an API change, even though it's fixing an unintentional previous API change.

@adrn adrn added this to the v3.1 milestone Oct 19, 2018
@adrn adrn requested a review from eteq October 19, 2018 01:47
@astropy-bot
Copy link

astropy-bot bot commented Oct 19, 2018

Hi there @adrn 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

Needs a changelog entry noting the API change.

The 3.0 change was also an API change in the name, right? That is previously it was representation? Maybe we should just change it back to representation, but add a keyword representation_type that raises a deprecation warning for 1 (and only one) major version?

@adrn adrn force-pushed the coordinates/fix-realize-frame branch from ff734dd to 11e3309 Compare October 19, 2018 19:34
@adrn
Copy link
Member Author

adrn commented Oct 19, 2018

In the interest of not over-engineering (since no one complained when this changed names in the last version) and creating more work for the future (deprecation), I opt for just switching the name back to the correct / pre-v3.0 name. I added an API change entry to the changelog!

@eteq
Copy link
Member

eteq commented Oct 19, 2018

OK, that's fair. For this case it's pretty straightforward since it's a single-parameter method so it's likely that no one uses the keyword version anyway.

That said, as our canary in the coordinates coal mine... @Cadair, as far as you know you never have frame.realize_frame(representation_type=...) anywhere, right?

@astrojuanlu
Copy link
Member

I suggested data in #7784, but representation is also OK I guess :)

@eteq
Copy link
Member

eteq commented Oct 24, 2018

Thanks for referencing #7784 - hadn't connected that with this mentally, but you're right it's the same thing, @Juanlu001 !

I see your point re: data. I think representation is only a good name if we do the other thing you suggested in #7784 of making it set the representation_type in realize_frame. Basically, by naming it data I think we imply that it shouldn't set the representation_type, while representation sorta does (since that's the distinction in the attributes on the class itself.

See my #7784 (comment) about this, though. If we agree on that as the thing to do is change the name to data, but if we instead opt for @Juanlu001's original suggestion in #7784 I think we should leave the name as representation.

@adrn adrn force-pushed the coordinates/fix-realize-frame branch from 11e3309 to 4d3d86c Compare October 24, 2018 21:28
@Cadair
Copy link
Member

Cadair commented Oct 25, 2018

This looks good to me. The SunPy tests all pass etc.

Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

The test failure in the docs looks real, I think because of the dangling * here?

astropy/coordinates/baseframe.py Outdated Show resolved Hide resolved
Co-Authored-By: adrn <adrianmpw@gmail.com>
@adrn
Copy link
Member Author

adrn commented Oct 26, 2018

Fixed!

@adrn adrn added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Oct 26, 2018
@mwcraig mwcraig merged commit 1586d7b into astropy:master Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coordinates 💤 merge-when-ci-passes Do not use: We have auto-merge option now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants