feat(api): publish traversed spec to gosling api #978
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -69,6 +69,19 @@ export function compile( | |||||
| trackInfos = getRelativeTrackInfo(specCopy, theme).trackInfos; | ||||||
| } | ||||||
|
|
||||||
| // publish the fixed spec | ||||||
| // used for alt-gosling | ||||||
| try { | ||||||
| if (PubSub) { | ||||||
| PubSub.publish('specTraversed', { | ||||||
| id: specCopy.id, | ||||||
| data: specCopy | ||||||
|
There was a problem hiding this comment. Maybe more intuitive and consistent with how we use "data" and "spec" in other API functions
Suggested change
There was a problem hiding this comment. Ah I see, I had already written this piece before the standardization! Thanks! |
||||||
| }); | ||||||
| } | ||||||
| } catch (e) { | ||||||
| // | ||||||
| } | ||||||
|
|
||||||
manzt marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| // Make HiGlass models for individual tracks | ||||||
| createHiGlassModels(specCopy, trackInfos, callback, theme, urlToFetchOptions); | ||||||
| } | ||||||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the name of the event, how about
specProcessedinstead? I think users might not understand what the traverse means. And, we can explain what is the different between the input spec and the processed spec briefly in the documentation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea! I indeed was fidgetting around with a good name, specProcessed is more intuitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, bikeshedding, maybe
specResolved?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like
specResolved, which I think gives a better impression that the spec is not converted into another format, but rather the spec is slightly modified to resolve things!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't resolved give the idea that the spec is fully changed into the 'final form' that it gets for gosling? Because that isn't the case here, it is just partially processed. I'm not sure what the convention is, but I'm afraid resolved gives off the wrong idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input spec is modified to have default values filled in, and some of the users' incorrectly-specified properties are fixed. Some parts of the spec are not touched by the program (e.g., when values are already specified by users, so no need to add default values), which I think is what @thomcsmits was referring to by
"it is just partially processed".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we don't emit a fully processed spec ("fully processed" as in the final state that it becomes in Gosling), but that might happen in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have truely strong opinions on this, so I'm happy to switch it to specResolved still
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#985 (as this PR is already merged)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a strong opinion about "resolved" vs "processed", but think this could be a good exercise to be explicit about the state of the gosling spec when it is emitted. "Resolved" and "processed" convey that completion of some transformations/resolution, which is not the case. I have left a longer comment in #985