Repository navigation
Add support for sorting table with non-mutable columns - #9549
Conversation
| try: | ||
| col[:] = new_col | ||
| except Exception: | ||
| self[col.info.name] = new_col |
There was a problem hiding this comment.
I tried setting to the new column in all cases but this caused problems with indexes which would require a deeper dive to understand / fix.
There was a problem hiding this comment.
OK - I think here having a solution is definitely much better than worry about perfection. Maybe a new issue, though?
hamogu
left a comment
hamogu
left a comment
There was a problem hiding this comment.
Looks good to me, but I leave formal approval to a table maintainer.
| new_col = col.take(indexes, axis=0) | ||
| try: | ||
| col[:] = new_col | ||
| except Exception: |
There was a problem hiding this comment.
could you make it to the specific exception?
| except Exception: | |
| except TypeError: |
There was a problem hiding this comment.
This exception can be raised by arbitrary user-defined mixin columns, so I have no idea what exception class they might raise in this case.
|
Looks good, one tiny comment on the exception |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This is great! I have one request for a more elaborate comment, but it is not essential, so approving.
| try: | ||
| col[:] = new_col | ||
| except Exception: | ||
| self[col.info.name] = new_col |
There was a problem hiding this comment.
OK - I think here having a solution is definitely much better than worry about perfection. Maybe a new issue, though?
b6394b9 to
084d434
Compare
|
I think I have addressed review comments. @bsipocz - we need to do something, anything about the changelog conflict mess. 😈 I just got hit with a conflict again. |
|
This is really unfortunate, I agree. I'll try to play a bit with the rebase github action, it may just address the biggest painpoints we have. (But just as with the json approach, one doesn't need to insert the new entry at the very bottom of the section 🤷♀ ) |
|
I thought new entries where supposed to be ordered by PR number. If that's not the case, things do get easier... |
|
No, there are no rules on ordering. Traditionally they are ordered by the order of merging, but that's that the same as the PR number, and the conflicts are coming from the fact that one usually adds the new entry at the bottom of a section and git is very careful to not auto-merge these, but flag them as conflict. The rebase bot should easily handle it. |
Fixes #9536
EDIT: Close #9537