Accept strings for the mode when finalizing staged data #2232
Conversation
ognyanstoimenov
requested review from
alexowens90,
willdealtry and
poodlewars
as code owners
| {"write", None}, | ||
| ], | ||
| ) | ||
| def test_finalize_staged_data(arctic_library_lmdb, input_mode, expected_append): |
There was a problem hiding this comment.
This is a good unit test but I think we should also have some integration tests that check we are actually doing the right thing.
The expected_append==None case should probably be split to its own test, rather than parameters in this one.
| mode : `StagedDataFinalizeMethod`, default=StagedDataFinalizeMethod.WRITE | ||
| Finalize mode. Valid options are WRITE or APPEND. Write collects the staged data and writes them to a | ||
| new version. Append collects the staged data and appends them to the latest version. | ||
| mode : Union[`StagedDataFinalizeMethod`, str], default=StagedDataFinalizeMethod.WRITE |
There was a problem hiding this comment.
I think lowercase write, append will match both Pandas and our QueryBuilder APIs better.
| @@ -1383,9 +1383,12 @@ def finalize_staged_data( | |||
| 2024-01-03 3 | |||
| 2024-01-04 4 | |||
| """ | |||
| if not (mode is None or isinstance(mode, StagedDataFinalizeMethod) or mode == "WRITE" or mode == "APPEND"): | |||
There was a problem hiding this comment.
Should validate against mode==None shouldn't you, what does it mean?
| @@ -1383,9 +1383,12 @@ def finalize_staged_data( | |||
| 2024-01-03 3 | |||
| 2024-01-04 4 | |||
| """ | |||
| if not (mode is None or isinstance(mode, StagedDataFinalizeMethod) or mode == "WRITE" or mode == "APPEND"): | |||
There was a problem hiding this comment.
if not isinstance(mode, StagedDataFinalizeMethod) or mode not in ("write", "append") is clearer to me, as it's closer to English
| @@ -1383,9 +1383,12 @@ def finalize_staged_data( | |||
| 2024-01-03 3 | |||
| 2024-01-04 4 | |||
| """ | |||
| if not (mode is None or isinstance(mode, StagedDataFinalizeMethod) or mode == "WRITE" or mode == "APPEND"): | |||
There was a problem hiding this comment.
Should validate that the enum value is either WRITE or APPEND as protection against someone changing the enum and us silently treating it as WRITE
Reference Issues/PRs
What does this implement or fix?
Monday ref 8645841959
Support strings for finalize staged data
modeargument in addition to enumAny other comments?
Checklist
Checklist for code changes...