-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add initial conversion script from Documentation.js to old format #6777
Add initial conversion script from Documentation.js to old format #6777
Conversation
@limzykenneth I hit a bit of a snag with detecting methods in ES6 classes correctly. It seems like in some cases like in |
@davepagurek Was just going to ask, would you like me to put together a simple test document so that you can compare against a smaller generated file instead of the full one? |
Are you thinking something like just a subset of p5? |
Yeah something like that, I'm extracting the p5.Framebuffer class at the moment to test things out. You can try it if you like: https://gist.github.com/limzykenneth/37b52c2bed719a211d7d9f24eb6d47cd |
I did some digging and found that the problem is likely we are labeling the class as I tried the |
@davepagurek In the gist I have updated the test file to something that works 90% of the way. The main thing that doesn't work at the moment is modules. I'm not sure with documentation.js it is entirely possible without slightly annoying configs though, we may need to consider the structure we use to get around this. |
Thanks for your pointers! Looks like I can get it to work when:
/**
* An object that one can draw to and then read as a texture. While similar
* to a p5.Graphics, using a p5.Framebuffer as a texture will generally run
* much faster, as it lives within the same WebGL context as the canvas it
* is created on. It only works in WebGL mode.
*
* @param {p5.Graphics|p5} target A p5 global instance or p5.Graphics
* @param {Object} [settings] A settings object
*/
p5.Framebuffer = class Framebuffer {
/**
* Description here
*/
begin() {}
} The So far I've just made the changes to |
I think the last things to do are to implement conversion of constants, and to update other classes in the p5 source code to the constructor syntax that works (and to document that somewhere.) Out of the few top-level properties in the old json format, there are two that I think are safe to omit:
|
These can possibly be omitted as I don't think they are being used. The bundled stuff I think is just the parameter info in the
I believe so, in any case we don't need to consider space saving for this file since it won't be served in the same way as the data.json file we have currently on the website. Feel free to include or omit them as you like. |
I was looking into consts some more. I think yuidoc logged usages by seeing where they get used inside functions, since we don't specify within param types (currently anyway) specific constants, just the general It looks like Documentation.js doesn't give us the same sort of info on internal usage. I think that maybe the thing to do instead is replace uses of |
I've been testing this on the p5.js-website repo by running One thing that remains, though, is that the ordering on p5js.org is different. @limzykenneth do you know how the ordering works currently? Is it just the order categories are added to the object here? https://github.com/processing/p5.js-website/blob/d311934b81eaed6b48161384493b14e3522f65cd/src/assets/js/reference.js#L4450 Do you think this is something we should try to match, or wait for the new site to use a different ordering method? |
@davepagurek I would need to look into the code in more detail but I think it may be related to the order things already are in in That being said, we don't necessarily need to match if we are not deploying this to the current website. We can decide later if we need to sort at this step or the website can sort instead. |
Comparing the current website's reference section order with the |
I've made some changes so that I could run the FES checks successfully. This includes:
With that, running Once the existing tests ran, I also add tests to make sure that in addition to the existing I also have a playbook for being able to update other test files, which mostly includes:
|
@davepagurek I just had a check on the converted JSON working with the current website and noticed that there are still some issues between them. The Constants section currently seems to be blank. The other probably more significant thing is that some functions related to event handling are missing (eg. |
If you wanted to have valid JSDoc, you could could add some
or more strictly
and then it's ok to use No idea...
|
Update on the constants section missing: the current reference page only shows constants with examples, and I had forgotten to add the examples and alt properties to constants. That bit should be good now. Going to look into the event handling ones soon! @lindapaiste I think our migration path will be something like:
|
Ok I found out what was going on with the event handling methods! Turns out it wasn't anything fundamentally wrong with the duplicate names, I just forgot to apply the rules from #6777 (comment) to that class too. Now its properties and methods show up correctly. |
Another update: I've added typedefs for all constants and replaced all usage of a generic For constants that @limzykenneth converted to symbols, right now I'm using |
@limzykenneth just resolved all the merge conflicts so that the docs stay in the expected updated format, lmk if this is good to merge! |
I'll merge for now and worry about it later if something breaks |
This adds some boilerplate for a node script to convert
docs/data.json
to the olddocs/reference/data.json
format. Run it by callingnode utils/convert.js
and it'll dump the output indocs/converted.json
.