Update entropy types #1915
Merged
Update entropy types #1915
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nextstrain-bot
temporarily deployed
to
auspice-victorlin-updat-jrdeox
genehack
approved these changes
Nov 21, 2024
jameshadfield
requested changes
Dec 17, 2024
victorlin
force-pushed
the
victorlin/update-entropy-types
branch
from
ce1f446 to
cad5eb6
Compare
genehack
reviewed
Jan 7, 2025
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. Notably, a type assertion in _frame is a necessary part of this translation. I considered extracting the modulus operation into a separate _mod3 function to reduce clutter in the _frame function, however that seemed to be an unnecessary complication.
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.
The function is only used in two places, both of which specify a valid strand value.
This is only used in one place, so use it directly.
Setting the nested dictionary value directly is more readable.
These types had ignored the reality that the JSON input can be anything. The removal better aligns with the validation that is already present in the functions. More validation has been added to accommodate for newly exposed type errors.
victorlin
force-pushed
the
victorlin/update-entropy-types
branch
from
0d9600b to
d3e89c0
Compare
jameshadfield
approved these changes
Jan 15, 2025
| "Note that -ve strand RNA viruses are typically annotated as 5' → 3'."); | ||
|
|
||
| if (!nucAnnotation) { | ||
| throw new Error("Genome annotation missing 'nuc' definition"); |
There was a problem hiding this comment.
[comment, no changes please] This made me think of how nice it'd be to have #1813
|
Thanks for the reviews @genehack and @jameshadfield! Merging this now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
Updates TypeScript usage to be consistent with #1864, etc.
I thought of these changes after adding 6215591.
Discussions
%3operationas Frameoras 0 | 1 | 2?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.