Update entropy types #1915
Update entropy types #1915
Conversation
This allows for more specific type narrowing.
Improves readability.
Improves readability.
This allows some code editors to show the comment on hover. I also added two new types (Phase, Frame) to replace some comments.
For consistency.
These were not consistent in the same file. I've also opted to not use them in the tree component types.
The previous error message code indicated the possibility that this property is unset. Express that directly in the type annotation and simplify the error message code.
Checking against undefined is more direct.
nextstrain-bot
temporarily deployed
to
auspice-victorlin-updat-jrdeox
| end?: number | ||
| start?: number | ||
| segments?: JsonSegmentRange[] | ||
| strand?: Strand |
There was a problem hiding this comment.
making sure this needed to become optional to match some usage somewhere?
There was a problem hiding this comment.
In 2d3a331 the matched usage was Object.prototype.hasOwnProperty.call(annotation, "strand") which to me indicates that it is optional. It's hard to tell from other usage because the entire JsonAnnotation object is represented by json.meta.genome_annotations here:
auspice/src/actions/recomputeReduxState.js
Line 819 in a7578d2
auspice/src/actions/recomputeReduxState.js
Line 870 in a7578d2
@jameshadfield might have a better idea.
There was a problem hiding this comment.
It's optional for the nuc annotation. You could argue it should be required, but we relaxed this and just assert that it's not negative:
auspice/src/util/entropyCreateStateFromJsons.ts
Lines 93 to 94 in c0ca42f
For CDSs to be considered valid it must be "+" or "-":
auspice/src/util/entropyCreateStateFromJsons.ts
Lines 187 to 188 in c0ca42f
At first, I disabled @typescript-eslint/consistent-type-assertions for the type assertion in the original code, but that ended up polluting the _frame() function with details unrelated to the direct purpose of the function. Extracting the modulus operation into a separate function and adding the details there felt like a better scope.
| // enum Strand {'+', '-'} // other GFF-valid options are '.' and '?' | ||
| type Strand = string; | ||
| // '.' and '?' are GFF-valid options | ||
| type Strand = '+' | '-' | '.' | '?'; |
There was a problem hiding this comment.
GFF spec allows '+' | '-' | '.' | '?' but our JSON schema doesn't use GFF spec here. There's some discussion I can dig up around this if you'd like (I wanted to use GFF spec) but Nextstrain uses None in python and null in JSON/JS instead of "." . Here's a salient comment:
auspice/src/util/entropyCreateStateFromJsons.ts
Lines 189 to 190 in c0ca42f
| function calculateStackPosition(genes: Gene[], strand: (Strand|null) = null):number { | ||
| function calculateStackPosition( | ||
| genes: Gene[], | ||
| strand: Strand | null = null, |
There was a problem hiding this comment.
Related to a previous comment, null is probably not the right choice here (as it's a schema-valid strand). By the time this function is called we have already created cds objects (cdsFromAnnotation) and so restricted the strand to +/-, so in practice it doesn't mater. I'd suggest
- (a) requiring the argument (and thus getting rid of the conditional
if (strand)below)
and, potentially,
- (b) using a more restricted type definition.
| @@ -20,7 +20,7 @@ interface JsonAnnotation { | |||
| end?: number | |||
| start?: number | |||
| segments?: JsonSegmentRange[] | |||
| strand: Strand | |||
| strand?: Strand | |||
There was a problem hiding this comment.
A broader comment / question about TS as it relates to the JSON dataset. Beyond being semantically valid JSON anything could be present in the JSON (we don't do formal schema validation within Auspice). So should any types relating to the dataset contents therefore allow any and every field to be optional, and also all values to be any type?
This relates to a7578d2, which proposes
- if (typeof annotation.display_name === 'string') {
+ if (annotation.display_name !== undefined) {(annotation here is direct from the JSON dataset). According to our schema display_name will always be unset or a string, but in reality it could be anything.
| return (positiveStrand ? | ||
| (start+phase-1)%3 : | ||
| Math.abs((end-phase-genomeLength)%3)) as Frame; | ||
| return positiveStrand ? | ||
| _mod3(start+phase-1) : | ||
| _mod3(end-phase-genomeLength); | ||
| } | ||
|
|
||
| /** | ||
| * Type-safe modulo operation. Return value is unsigned (i.e. non-negative). | ||
| */ | ||
| function _mod3(n: number): 0 | 1 | 2 { | ||
| if (!Number.isInteger(n)) { | ||
| throw new Error(`${n} is not an integer.`); | ||
| } | ||
|
|
||
| /* TypeScript cannot infer the exact range of values from a modulo operation, | ||
| * so it is manually provided. | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| return (Math.abs(n) % 3) as 0 | 1 | 2 |
There was a problem hiding this comment.
For what it's worth, I find these changes kind of unfortunate (I have no doubt they are just to appease tsc). I'd prefer expanding out the ternary return into a longer function with more comments than additional small functions. I like the cast the best!
The lack of an integer type is pretty frustrating (JS, not TS, to be fair). If the numbers supplied to this function aren't integers we've got bigger problems in Auspice than a floating point frame!
Description of proposed changes
Updates TypeScript usage to be consistent with #1864, etc.
I thought of these changes after adding 6215591.
Checklist
If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR(to be done by a Nextstrain team member) Create preview PRs on downstream repositories.