schemas: Add missing display_default properties for Auspice config v2 #916
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
tsibley
force-pushed
the
trs/schema/missing-auspice-display-defaults
branch
from
eed47a8 to
0c09991
Compare
jameshadfield
requested changes
May 12, 2022
There was a problem hiding this comment.
Changes look good from reading the diff. Scanning the languages auspice supports I think they're all BCP-47, although this was never specified explicitly.
CI is failing, though it's not immediately obvious why...
ERROR: 'nDescendents' is not one of ['clade', 'aa', 'none']. Trace: ... - properties - display_defaults - properties - branch_label - enum
…g and dataset) Resolves <#911> which noted "language". In confirming that, I cross-referenced Auspice's view settings doc¹ and code² which revealed "sidebar" and "panels" too. Clarifies the purpose of the two different "panels" keys. ¹ https://docs.nextstrain.org/projects/auspice/en/stable/advanced-functionality/view-settings.html#dataset-json-configurable-defaults ² https://github.com/nextstrain/auspice/blob/ce8380f7/src/actions/recomputeReduxState.js#L699-L718
…nd dataset
Reconciles the two schemas with some minor improvements + a bug fix.
While the enum values in one schema were valid, they're not the _only_
valid values.
This display_default refers to the keys of branch_attr.labels objects on
nodes, so I reused the former's pattern of allowed characters. I think
that pattern may be too restrictive and not accurately describe what
Auspice will handle, but that's a broader issue with other parts of the
schema too and something to investigate for another day.
Hint that there's a special-cased value ("none") by mentioning it
explicitly in the pattern.
tsibley
force-pushed
the
trs/schema/missing-auspice-display-defaults
branch
from
0c09991 to
65ebea1
Compare
|
Sorry, yeah, wasn't quite ready for review. Auspice langs should be BCP-47 (and appear so to me). Repushed with a fix addressing the schema for |
Codecov Report
@@ Coverage Diff @@
## master #916 +/- ##
==========================================
+ Coverage 34.66% 35.02% +0.36%
==========================================
Files 42 42
Lines 6018 6189 +171
Branches 1542 1605 +63
==========================================
+ Hits 2086 2168 +82
- Misses 3859 3940 +81
- Partials 73 81 +8
Continue to review full report at Codecov.
|
jameshadfield
approved these changes
May 12, 2022
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.
Resolves #911 which noted
"language". In confirming that, I cross-referenced Auspice's view
settings doc¹ and code² which revealed "sidebar" and "panels" too.
Clarifies the purpose of the two different "panels" keys.
¹ https://docs.nextstrain.org/projects/auspice/en/stable/advanced-functionality/view-settings.html#dataset-json-configurable-defaults
² https://github.com/nextstrain/auspice/blob/ce8380f7/src/actions/recomputeReduxState.js#L699-L718
Testing
Not yet tested. Will look at CI tests and maybe add a test case too.