Improve reproducibility of the column reordering operation #7227
Conversation
This implements the two criteria defined in OpenRefine#5576 concerning the behaviour of the operation when: * only reordering columns, without deleting any * only deleting columns, keeping the order of the remaining ones Those changes can be made without changing the dialog's UI. Making the operation analyzable in general would likely require a different UI to let the user better specify their intent.
SoryRawyer
left a comment
SoryRawyer
left a comment
There was a problem hiding this comment.
@wetneb apologies for the delay reviewing this! Just to confirm, this is somewhat of a breaking change in that columns will no longer be removed in every case. Is that right? I know that's what the user feedback gathered by @magdmartin states, but if and when this is released I want to make sure to point this out in the release notes.
Also, it looks like there are reorder-columns commands in other frontend components as well: menu-edit-column.js and data-table-view.js also have reorder commands in them. Should those be updated as well (even if only to avoid having a null value for isPureReorder)?
| ColumnReorderOperation op = ParsingUtilities.mapper.readValue("{\"op\":\"core/column-reorder\"," | ||
| + "\"columnNames\":[\"b\",\"c\",\"a\"]}", ColumnReorderOperation.class); | ||
| assertEquals(op._columnNames, List.of("b", "c", "a")); | ||
| assertEquals(op._isPureReorder, false); |
There was a problem hiding this comment.
@SoryRawyer The test above shows the backwards-compatibility for this change. The old serialization format of this operation (without the isPureReorder field) will still be understood as before, as an operation that removes any columns that are not mentioned in the columnNames list.
This means that if users execute recipes generated from a previous version of OpenRefine with the new version, they should still get the same results.
That's a very good point! The column reordering operation that is used to implement the "join" action is one that is used to remove columns only, without doing any reordering. Thanks to the backwards-compatibility mentioned above, it doesn't need a |
Thanks for the context! I don't think that's necessary to include in this PR. I think this looks good! I'm going to approve but wait a day or two to merge in case anyone has a last-minute comment. |
2 participants
This implements the two criteria defined in #5576 concerning the behaviour of the operation when:
Those changes can be made without changing the dialog's UI. Making the operation analyzable in general would likely require a different UI to let the user better specify their intent.
I am not sure if it should close the issue, maybe it's good enough, maybe not.