Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TwoDimensionalPlotLine data type so the pointStyle member is not … #624

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jayatvitskiy
Copy link

@jayatvitskiy jayatvitskiy commented Apr 23, 2021

…a union. Resolve #613

This is my first contribution to Webviz, so please let me know if I'm missing anything and/or if there's anything I should seek to do better in the future!

Summary

This is a very small pull request aimed to resolve issue #613.
As pointed out in the issue, the 2D Plot Template for Node Playground does not presently compile. This is because the return value is a TwoDimensionalPlot object. A TwoDimensionalPlot contains multiple TwoDimensionalPlotLine objects, which each have an optional member called pointStyle. Prior to my change, pointStyle was a union data type. The AST parser throws an error when we try to return an object that contains a union, so the template wasn't compiling.

I observed that there's no need for pointStyle to be a union type. In Typescript, a union type allows us to say pointStyle may be type X or type Y, but pointStyle is actually always a string. Thus, I simply changed each instance of pointStyle in the code so that instead of being a unique union data type, it would simply be a string.

In fact, I checked the GitHub blame feature for the relevant files and observed that pointStyle was actually previously a string (it was changed to a union 6 months ago, which is presumably what broke the template).

Test plan

I ran npm test to ensure that I didn't break anything. I observed that the current master branch does not actually pass all the tests, but my code passes the exact same number of tests as the master branch. Thus I concluded I did not break any tests.

I opened the 2D plot template. I no longer receive a compile error, and the template works as intended, so I believe I successfully addressed the issue in #613.

Next, I wrote a custom NodePlayground node that returns a TwoDimensionalPlot:
Screen Shot 2021-04-22 at 11 58 13 PM

My goal here was to determine whether changing pointStyle to a string broke the intended functionality of pointStyle (i.e. am I still able to change the points on my plot to be stars, rectangles, etc). The above code resulted in a 2D plot with star indicators at (1,2) and (3,5) and a line between. I tested other point styles as well, such as "rect", and they worked correctly. Thus I concluded that the pointStyle functionality was working as intended.

Screen Shot 2021-04-22 at 11 57 06 PM

Versioning impact

N/A

@lusuwen
Copy link

lusuwen commented Sep 14, 2022

using code here, I receive a compile error "Unions are not allowed in return type." and i have no idea

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2D Plot Node Template: Unions are not allowed in return type
2 participants