Skip to content

Allow renaming mixin columns - #5469

Merged
mhvk merged 1 commit into
astropy:masterfrom
taldcroft:table-rename-mixin
Nov 11, 2016
Merged

mhvk merged 1 commit into
astropy:masterfrom
taldcroft:table-rename-mixin

Conversation

@taldcroft

Copy link
Copy Markdown
Member

This was a leftover to-be-done from the original mixin implementation.

@larrybradley

Copy link
Copy Markdown
Member

Thanks, @taldcroft!

@taldcroft

Copy link
Copy Markdown
Member Author

@mhvk - I think this is good to go.

I played a bit with an alternate implementation which used a name property in the MixinInfo class. But it somehow got messy and I got into a whack-a-mole with different odd problems showing up in tests. So the current implementation that encapsulates this specific behavior in __setitem__ is not my first choice, but I do think that it is correct and reliable.

@mhvk

mhvk commented Nov 11, 2016

Copy link
Copy Markdown
Contributor

@taldcroft - this looks good, so I'll merge.

p.s. Had you not addressed it above, I would indeed have wondered why a name property would be an idea, mirroring BaseColumn...

@mhvk
mhvk merged commit 1726a90 into astropy:master Nov 11, 2016
@taldcroft
taldcroft deleted the table-rename-mixin branch February 25, 2019 20:24
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.

3 participants