Hi there @eteq 👋 - 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 labelled 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! 👍

@eteq - it should be possible to do this more easily - for things like slicing, similar problems were found, and this was solved by keeping track of attributes that are not part of the current frame in _extra_attr_names - see, e.g., _apply. So, rather than go over all names, one can just copy over the frame attributes and those extra ones (I'm sorry I missed that in #5751). I think it actually makes sense to follow the precedent of _apply here too.

Separate but related: for some of our packages, the design has now become sufficiently complicated that I think it might start to make sense to explicitly document design - @astrofrog suggested that in #4821 for modelling (but sadly discussion of where to put it meant that PR was never merged...). This is also useful for users who want to understand the internals.

Thanks for pointing that out @mhvk - definitely better to use that! The commits I pushed up do that, as well as fixing some problems travis failures revealed. (as a related aside... I'm looking forward to not having to support py 2.x any more 😉)

@mhvk, also note the last commit (2c458f1), which is a cosmetic change that I think makes it a bit easier to understand what _extra_attr_names (renamed _extra_frameattr_names) is for. Do you agree?

the design has now become sufficiently complicated that I think it might start to make sense to explicitly document design

Quite possibly true. Coordinates has this to some extent in the form of the concept overview, and the coordinates APE but perhaps that's less technical then what you had in mind? The big concern I have with that sort of technical docs is then you have to keep it up-to-date when the code changes, so arguably it's better to use comments for this sort of thing. But there's certainly a happy medium where very-unlikely-to-change bits end up in design docs.

👍 on clarifying the name. But, with no intent to bikeshed, maybe make it _extra_frame_attr_names in analogy with, e.g., get_frame_attr_names. Indeed, while you are at it, maybe replace with frame_attrs also the use of frameattrs in __repr__ and of frattrs in BaseFrame._apply...

OK, will now look at the functional side...

Separately, on a "design document": I indeed was thinking about technical implementation more than overall design. I agree that the risk of any description not being updated is large, and in that respect like the original idea of having it be close to the code (but typeset by sphinx). One perhaps not so silly option is to start using the docstring of each module/file for this purpose (think of the README.md in #4821 being the docstring in __init__.py instead. I'll make a comment there...

p.s. The failure looks real but unrelated.

LGTM as well - merge if you're happy @eteq

Is this related in a way to #6462?

Did a quick merge commit - assuming all passes after that, this is ready to merge - whoever sees it first (and has rights) should feel free to hit the button.

Re #6448 about the "design document": I like that idea of doing it in the docstrings! It's still potentially worth putting some things in the docs, but I think we want to try to restrict that to things we think are over-arching or require complex diagrams or similar. Maybe we should have an issue to track this(i.e. either docstring or doc tech notes as appropriate), @mhvk?

@eteq - don't we normally rebase to get rid of merge commits?

@eteq - @astrofrog is right, could you please rebase?

@eteq In addition to rebasing to remove the merge commit, can you please fix this failing PEP8 test:

astropy/coordinates/tests/test_regression.py:559:36: W292 no newline at end of file

Rebase + blank line done.

@astrofrog @bsipocz - The reason for the merge commit being there is that this is what github does when you use it's in-web auto-resolve feature. If it hadn't been for the carriage-return issue, this would have been fine. So I think we should re-consider the "always rebase" policy given how much easier it is to merge things directly from github now with the in-browser conflict resolution.

Maybe a discussion item for the CC mtg in a few weeks?

@eteq:

Maybe a discussion item for the CC mtg in a few weeks?

Sounds good - can you add it to the list of topics?

Also can you comment on whether this has any relation to #6462

So I think we should re-consider the "always rebase" policy given how much easier it is to merge things directly from github now with the in-browser conflict resolution.

maybe we need to try to convince github to add a similar button with a rebase on master functionality. I've asked this as a feature already, a long long time ago, maybe if there are more than us it will be implemented.

@bsipocz:

Maybe worth trying out on a test repo?

as I recall that did a mess, too, can't remember though what it was (e.g. merge blindly after a rebase is only useful in a few very clear situation). Definitely a +1 to discuss at CC mtg.

Merging now as this got approved and no need to wait for the osx build (the rest is passing).

Thanks @eteq!

@astrofrog - Sadly that rebase option is not even available when there is a merge conflict: