Document overloaded methods nicely

[toolness]

This is a work-in-progress pull request, please don't merge it yet.

This is an experimental proof-of-concept to see how feasible it may be a PR to document overloaded methods in a way similar to Java Processing's documentation, as per @lmccart's explanation in #1287 (comment).

Here's what the new p5.color() docs look like in this PR:

The good news is that YUIDoc already has a mechanism for recording information about overloaded methods: if you define a method twice with different params, it just adds both versions to data.json. We just need to add logic to our view layer to ensure that our index page doesn't contain a separate link for each overloaded version, and that the item detail view properly lists all the different method signatures and all the params. That's what this PR attempts to do.

Limitations:

• This PR makes things work with overloaded methods, but it may not work with overloaded constructors or other kinds of callables.
• Ideally I think that all the optional params would be listed after all the required params. Shouldn't be hard to fix.
• Unlike the original Processing docs for color(), I documented [alpha] as an optional param, which resulted in only two different signatures rather than four. There's no reason we can't document it the way Processing does--I was being a bit lazy, I guess. 😁
• A lot of logic for the overloading is done during render time, rather than during build time. It'll be better for QA to raise errors and such at build time instead. I'm thinking that we could do some kind of transformation to data.json, maybe, so that the item view just displays information rather than doing a bunch of the logic I've added in this PR. This will also probably make the information easier for the experimental type-checker and friendly error system to use, too.
• I'm not really sure how much I like the whole "just document multiple versions of the same method in separate comment blocks" approach of YUIDoc. As can be seen from the example of p5.color() that I've done for this PR, it reads a bit weird to have a comment block with just one version of the method's params and tons of examples, and then have little overloaded versions documented in separate comment blocks right after it. We might be able to experiment with this. But another alternative, if we really don't like this approach, may be to invent our own syntax for describing overloaded methods and bypass YUIDoc entirely.

[toolness]

Because of the way the parameter merging logic works, we don't need to actually describe [alpha] because we've already documented it in the previous version of the method.

[toolness]

I've added some assertions to the preprocessor that actually enforce that the same params have the same type, and that subsequent declarations of the same param have an empty description, among other things. This may be a bit draconian, but for now I wanted to make sure that the expectations of the overload code are well-understood.

I think it can also help prevent confusion--for instance, if a contributor were to provide a different description for the second version of alpha and the docs build system didn't complain, the contributor might then get confused when they don't see their description anywhere in the docs!

[toolness]

@lmmcart, do you have any thoughts on the way overloaded methods are being documented in this PR (i.e. via multiple comment blocks for each signature)? If that style of doing things looks OK to you, I can go ahead and polish up this PR for review. Otherwise I can try to figure out a different way of documenting them...

[lmccart]

Wow I don't know how I missed this until now, this is awesome, and makes the documentation soo much more understandable. I think the multiple code blocks are fine, especially if there are bigger fish to fry right now. We can always revisit later if it's posing significant readability / workability issues.

I'm fine with the optional [alpha] param shortcut... I think this could actually be a more easily understood and readable version than the alternative of spelling out every variation.

We do have issues already with render of reference being quite slow (processing/p5.js-website#143), so anything we can do to shift time intensive steps to build stage, or reduce the size of data.json or reference.js (as @limzykenneth mentions) is great.

[toolness]

Yay! 😁

So I found out about something called a preprocessor for yuidoc. It's not very well-documented, but after looking at a real one it appears to just be a JS module that takes a parsed data.json-style object and changes it in whatever way it sees fit. Then yuidoc writes the resulting object to data.json. So we can totally use this to do a lot of logic at build time, as well as do some validation to ensure that our docs are correct!

I've added such a preprocessor in de8d4f3, though the code still isn't as clean as I'd like it to be, so I might fiddle with it more. I'd also really like to add a unit test for the preprocessor--partly to ensure that the preprocessor actually works, but also to serve as documentation, since otherwise it's not entirely clear how we're changing the structure of data.json for overloaded methods.

[toolness]

I'm wondering if we should leave the square brackets off optional parameters in the parameter documentation (not the syntax section). For instance, if alpha actually isn't optional in one of the overloaded signatures, it doesn't make sense to put brackets around it in the parameter list... I guess we could add some logic to only put brackets around it if it's optional in all overloaded signatures, like color() currently is in this PR. Bit of a weird edge case but I want to make sure it doesn't confuse anyone.

[toolness]

Er, do we have any purely node, non-browser-side tests at the moment? I guess I might have to add a new node-side mocha suite to test the preprocessor...

[toolness]

Ok, I think I'm happy with this. Aside from my question about leaving off brackets in parameter docs I think this is good to go...

[limzykenneth]

Having just replied to #1324 I think we need to have clearer ways to indicate a parameter is optional, square brackets doesn't mean much by itself I think. As mentioned by @lmccart in #1287 and here the Processing reference have quite a nice way to handle multiple ways a function can accept parameters by listing them all so I guess no square brackets is better.

[toolness]

Hmm, I agree that the square brackets convention is actually pretty unclear for beginners. That said, @lmccart did mention above:

I'm fine with the optional [alpha] param shortcut... I think this could actually be a more easily understood and readable version than the alternative of spelling out every variation.

I'm not sure how complex p5 API signatures get, but if there are calls with lots of optional parameters, it could get really long to actually list out every possible permutation without using square brackets. And since square brackets is a common convention used in other APIs/libraries/command-lines/etc, it makes me wonder if we should instead just make it really easy for readers to learn what the square brackets mean, rather than avoiding them entirely?

Some other options, not mutually exclusive:

1. Have a little blurb under the syntax signatures that explains what the square brackets mean. It is only displayed if square brackets are actually used in any of the syntaxes.
2. Add a hover tooltip over parameters in square brackets, explaining what the brackets mean. (We could also actually include the descriptions for the parameters on hover too.)

[limzykenneth]

@toolness Yeah I was thinking about the various permutation of the possible function calls as I type that as well.

Would it be better if under the parameter list, in addition to the square brackets in the syntax list, we have something like:

v3       Number: blue or brightness...(etc)

alpha    [optional] Number: alpha value...(etc)

[toolness]

Ah yeah, I like that!

My only concern then is the problem I was trying to describe in #1295 (comment) but probably failed miserably at, which is: in your example, what if alpha is optional in one overloaded function signature but required in another? One solution is to simply prohibit this from ever being the case, so that we raise an error at documentation build time if it's detected. I don't know enough about the p5 APIs to know whether that's a reasonable constraint or not, though.

Does the problem I'm describing make sense?

[limzykenneth]

Ahh...I think I misunderstood you a bit and yeah good point about that. Is there a particular case that you remember in the p5 API of that being the case? How is it documented now? I'll do some digging as well.

[toolness]

Cool, thanks!

I don't know of a particular situation in which that's the case--it was just my "must handle every possible edge case" OCD kicking in that made me think of it :)

[limzykenneth]

Just had an idea: we could have something like optional group of parameters. Take for example reference for arrayCopy(), there are two forms to this function either with 2 parameters or all 5. The optionals in the parameter list can be grouped together as one optional group, something like:

src Array: the source Array
dst Array: the destination Array

Optional 1:
srcPosition Number: starting position in the source Array
dstPosition Number: starting position in the destination Array
length Number: number of Array elements to be copied

(Maybe not done like this but that's the idea)

So it means you cannot leave out length when you are using srcPosition and dstPosition. Make sense? What do you think? 😉

[lmccart]

@limzykenneth in the case where there are several optional params that need to be used together, I think we could just display a different signature documenting this, similar to what we see in @toolness's image here.

[lmccart]

@toolness regarding the square brackets, I think both of those solutions make sense. Another idea, maybe less explicit, but more clear than currently, is just to add something to yuidoc build that adds "(optional)" to the description in the parameter listing if there are square brackets.

[alpha] Number: (optional) alpha value relative to the current color range

[lmccart]

👍

[limzykenneth]

Yeah that does work quite nicely. Just wondering the issue raised here if it would ever be an issue? For example if

color(gray, [alpha])

alpha isn't optional but for

color(v1, v2, v3, [alpha])

alpha is optional as @toolness was mentioning how should we best document it? That OCD is rubbing off on me now I feel @toolness 😆

[lmccart]

This looks great! I felt this was stable and already significantly more helpful that we can at least merge this base in and continue discussion about further refinements if we'd like. I posted two examples of it in practice live to p5js.org ref:

• http://p5js.org/reference/#p5/background
• http://p5js.org/reference/#p5/color

Thank you @toolness and @limzykenneth for your input, this makes things so much clearer!! I am really excited about this. :)

[lmccart]

I also took a pass at adding this to the wiki page for inline documentation. Feel free to edit as you see fit.
https://github.com/processing/p5.js/wiki/Inline-documentation#additional-signatures