-
Notifications
You must be signed in to change notification settings - Fork 16
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
Implement tooling to generate multiple plotly schemas #18
Implement tooling to generate multiple plotly schemas #18
Conversation
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.
Thank you very much for the contribution!! ❤️
I've left a few comments with some questions 😄
@MetalBlueberry |
Okey, that last commit is an attempt to reduce the number of changes on this PR. I don't think we have to update go.mod version as the code doesn't require any new feature. It is good to leave it as is and only update dependencies if we identify a vulnerability or we need some functionality. The idea is that this will allow more people to use the library without worring about updating things. Also, In the readme you add instructions to bump to a specific version, Do you think we could write that in a way that just instructs people how to update to ANY version? The process should be the same. Last but not least, I tried to generate the code from this branch and I noticed the area_gen.go file is not generated. I couldn't investigate why it happen nor if it is expected, it just looks odd as I would expect all the previous types to remain valid. I will look into this once I find time for the project. Thank you for the contribution!! |
it is not generated, because it is not part of the json schema:
These are the new trace types:
Or via jsonviewer: If this update goes through, I'll submit the next update to the latest version, watching out for your current comments. I also plan to add dark theme, as in plotly for python I added a downloader script a description how to generate the new files without manual work, only using the scripts. Hope this helps to get it through the finish line :) |
74e8195
to
8414419
Compare
Alright! I added a few steps to the CI to automatically check that the scheme is correctly built and I've identified some bugs with consistency on the scheme generation 😅 Not perfect yet, But I've merged those changes here so we can get better feedback on your changes. |
Thanks a lot @MetalBlueberry |
This PR is making me think about how can we support multipe plotly versions? It is true that generating a new schema is straight forward, but it makes it hard for people just wanting to import the package an use it. One idea that crossed my mind, given that you already wrote a script to download the schema from plotly site, we cloud write a script that downloads multiple schemas to a directory in this repository and then pair it with the generate script so we can generate all the schemas at once. The proposal is to have a file strucuture that looks like
This way, any user can import the latest version of this library but still use any plotly version of their choice. I don't think we have to support all versions, for a first approach I'll sugest to just support 2.29 and 2.18. So the task you can do is. TaskWrite a script that reads a file in the root of the repository with a list of versions that download schemas to a directory called The file will be
Design decissions
Keep in mind this is a suggestion, feel free to sugest any improvements or make changes. This will remove the need to add instructions to the readme about how to run the generator and simply it to |
53c1fe9
to
ee89afd
Compare
@MetalBlueberry Also updated the readme now and added some release notes.
We've come a long way, thanks for your comments, really cool stuff, hope we can get this merged. and have next steps implemented in nxt PRs soon. |
31ee0df
to
c03de9a
Compare
c03de9a
to
2a3e744
Compare
@MetalBlueberry one would also need to make sure to use the correct wasm_exec.js but this again is a topic for another PR. |
2ebfeb6
to
a0eed3d
Compare
a0eed3d
to
da906d6
Compare
2b38ea9
to
14b3ee7
Compare
14b3ee7
to
c85ba65
Compare
@MetalBlueberry |
@MetalBlueberry |
Alright, I made a few tweaks.
If you are happy, I'm happy with this. We could merge it |
Thanks. One small cleanup comment, and then let's merge it. |
Let's Go!! |
No description provided.