fix: 73 - add 2.0.0 to the recognized FE versions #74
Conversation
Codecov Report
@@ Coverage Diff @@
## main #74 +/- ##
=======================================
Coverage 71.72% 71.72%
=======================================
Files 126 126
Lines 10030 10030
=======================================
Hits 7194 7194
Misses 2836 2836
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| const correctVersion = | ||
| ["1.0.0", "1.1.0"].indexOf(corporaProps?.version?.corpora_schema_version) > | ||
| -1; | ||
| const correctVersion = checkValidVersion(corporaProps); |
There was a problem hiding this comment.
prefer prefix with is 🙏 const isCorrectVersion = ...
There was a problem hiding this comment.
or maybe const isValidVersion = checkValidVersion(corporaProps); ?
| @@ -0,0 +1,11 @@ | |||
| export function checkValidVersion(corporaProps: any) { | |||
| const isValidVersionOne = | |||
| ["1.0.0", "1.1.0"].indexOf(corporaProps?.version?.corpora_schema_version) > | |||
There was a problem hiding this comment.
super nit:
Hoist constants out of the function declaration, so we don't need to create new arrays every time:
const VERSION_ONES = ["1.0.0", "1.1.0"];
const VERSION_TWOS = ["2.0.0"];
export function checkValidVersion(corporaProps: any) {
if (!corporaProps) return false;
const { version, schema_version } = corporaProps;
const isValidVersionOne = VERSION_ONES.includes(version?.corpora_schema_version);
const isValidVersionTwo = VERSION_TWOS.includes(schema_version);
return isValidVersionOne || isValidVersionTwo;
}
There was a problem hiding this comment.
When the CXG is created, why isn't the Schema 2.0.0-provided title being put into the CXG at cxg_group_metadata | cxg_properties | title? It would seem cleaner to simply read it from a single CXG location unconditionally. More generally, it would be cleaner if Explorer never needed to care about the Schema version and instead could just rely upon the CXG file version. So I'm questioning whether this fix should actually be a Data Portal CXG transformer fix. Are there other places in the Explorer that we the Schema version is depended upon?
(Similarly, when we fix the missing X_approx_dist property, it would be best if we added this to cxg_group_metadata | cxg_properties, rather than under the cxg_group_metadata | cxg_properties | corpora dictionary, to avoid coupling CXG and H5AD Schema versions).
|
@atolopko-czi The title is there. I believe this check was to prevent legacy datasets (the |
|
Discussed w/ @ebezzi that we should in fact be able to eliminate h5ad schema version checks in this code, but we'll merge this as is for. I'll file a followup story to replace the version check with a robust read of |
|
@tihuan Shouldn't this have typed the files? |
Hmm what do you mean specifically? What needs to type what files? |
|
@tihuan I thought our stance going forward with TS was that any changed files would also check in typings for the files at some level. In this case at the least We're actually intoducing untyped code in this PR |
|
Ahh I see what you mean now! Thanks for bringing this up 👍 Thank you! |
|
Follow up PR: #81 Thank you! |
Reviewers
Functional:
@tihuan
Issue #73
Changes