refactor: Replace usage of Column.index with column name#1126
refactor: Replace usage of Column.index with column name#1126bmingles merged 9 commits intodeephaven:mainfrom
Conversation
bmingles
force-pushed
the
965-remove-column-index
branch
from
66acca3 to
fab60f7
Compare
bmingles
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1126 +/- ##
==========================================
+ Coverage 43.40% 43.41% +0.01%
==========================================
Files 435 435
Lines 32682 32688 +6
Branches 8240 8244 +4
==========================================
+ Hits 14185 14193 +8
+ Misses 18448 18446 -2
Partials 49 49
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| return dh.Table.reverse(); | ||
| } | ||
| const column = IrisGridUtils.getColumn(columns, columnIndex); | ||
| const column = IrisGridUtils.getColumnByName(columns, columnName); |
There was a problem hiding this comment.
This is correct, but we're going to need a migration strategy for data that was stored in the old format (testing against Enterprise). Can just check against the sort value to see if columnName is provided, or instead define the DehydratedSort type as column: ModelIndex | ColumnName, then have a typeof check and look up the column name if a ModelIndex is provided.
An example is how we handle movedColumns:
@mattrunyon implemented that migration behaviour in https://github.com/deephaven/web-client-ui/pull/642/files (though it should be typed to a
ColumnName instead of a string).
There was a problem hiding this comment.
The movedColumns wasn't really a migration I'd say. It was adding behavior (support for moving ranges in 1 move), not changing or removing any. This issue removes using ModelIndex for the sort column.
We'll still need to make sure the ModelIndex order is saved since we rely on that and not names for many things still. I'm not sure if that is accurate, but this ticket does remove the direct uses of column.index from the API. We still have some reliance on the column order being maintained by the engine which is a bigger issue to deal with.
There was a problem hiding this comment.
I've updated the code to support index or column name when importing a sort, but exporting uses column name.
| export type DehydratedSort = { | ||
| column: ModelIndex; | ||
| column: ColumnName | ModelIndex; |
There was a problem hiding this comment.
I think we should make this LegacyDehydratedSort or something since moving forward we don't want DehydratedSort to use ModelIndex as the column. Then hydrate can take LegacyDehydratedSort and dehydrate return DehydratedSort without the | ModelIndex
There was a problem hiding this comment.
@mattrunyon I've added the LegacyDehydratedSort interface as suggested
bmingles
force-pushed
the
965-remove-column-index
branch
2 times, most recently
from
230a141 to
f120043
Compare
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
We need to mark this as a Breaking Change, since the TableUtils API is changing to only take a string for column name.
To mark it as a Breaking change, add the BREAKING CHANGE: footer to the PR description as described in the Contributing guidelines.
| expect(exportedSort).toEqual([ | ||
| { column: 3, isAbs: false, direction: 'ASC' }, | ||
| { column: 7, isAbs: true, direction: 'DESC' }, | ||
| expect(exportedSort).toEqual<DehydratedSort[]>([ |
There was a problem hiding this comment.
We should also add a test that it re-hydrates a LegacyDehydratedSort as well.
| [sortList.hasThree, 'name_999', 2], | ||
| ]; | ||
|
|
||
| it.each(testCases)( |
There was a problem hiding this comment.
TIL. Didn't know there was a it.each. Could have used this in a bunch of other cases.
There was a problem hiding this comment.
@mofojed There's also a string literal syntax supported by it.each that can be used to inject test variables. I have rarely used it but worth checking out the docs. https://jestjs.io/docs/api#2-testeachtablename-fn-timeout
There was a problem hiding this comment.
That's pretty neat! Good to know.
bmingles
force-pushed
the
965-remove-column-index
branch
from
f120043 to
4acdaa8
Compare
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
Still need to update the PR description with the BREAKING CHANGE footer. We need to either maintain API compatibility with things exported by our packages (in this case, the functions changed in TableUtils), or mark it breaking.
bmingles
changed the title
bmingles
changed the title
3 participants
resolves #965
BREAKING CHANGE: Removed index property from dh.types Column type. IrisGridUtils.dehydrateSort now returns column name instead of index. TableUtils now expects column name instead of index for functions that don't have access to a columns array.