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

Inconsistencies in jsDoc comments #171

Open
ff6347 opened this issue Feb 19, 2017 · 29 comments
Open

Inconsistencies in jsDoc comments #171

ff6347 opened this issue Feb 19, 2017 · 29 comments

Comments

@ff6347
Copy link
Member

ff6347 commented Feb 19, 2017

While going through the result of the output of documentation.js and comparing it to the comments I found some inconsistencies in the comments we need to fix. For example there are some comments where a return value is an Array of things. Written like this:

* @return {Stories[]} Array of Stories.

In other cases it is just:

* @return {Array} Array of strings

we got some:

/* todo */

Which are (I guess todo for a long time)

There should be an combined effort to review the comments on functions. Maybe by assigning files to contributor. Also we should write down/define how a new function should be documented.

@b-g
Copy link
Member

b-g commented Feb 20, 2017

@fabianmoronzirfas happy to help! Should we split the files by 3 or 4 (if @ffd8 is in too)?

This is looks better for me ... but I'm happy to follow any recommendation.

* @return {Stories[]} Array of Stories.

@trych
Copy link
Contributor

trych commented Feb 20, 2017

I think we also need to decide how the doc should look in the end. This should help us to come up with a consistent version for the inline docs.

From the Skype call, I remember that we want to aim for something close to p5.js. I think a good function to start from is something like b.color(), that has a few different combinations of possible arguments.

I'm super busy the next two weeks, could only look into it afterwards, maybe somebody is willing and has the time to come up with something before that?

@b-g
Copy link
Member

b-g commented Feb 20, 2017

@trych Good call! But can we do this as 2nd step and first fix things e.g. missing and wrong parameters etc. And then look into the "text" and examples?

@ff6347
Copy link
Member Author

ff6347 commented Feb 20, 2017

If we @ffd8 is with us splitting the files into 4 would be good. I think there is no hurry ( @trych ) action needed. I can work with the current state. I just wanted to point that out.

@b-g

This is looks better for me ... but I'm happy to follow any recommendation.

  • @return {Stories[]} Array of Stories.

I'm totally fine with whatever we come up with. It just should be consistent.

Another thing that I saw is that we also have sometimes a Collection as return value. This should also be reflected in the statement. Like

* @return {Stories} Collection of Story elements  

Or something like this.

@trych My aim is the following:

index page with all the functions and then a sub page for each function. The subpages need a lot of love. I can generate them but if we want to add further infos we need to write this by hand.

What we can do is automagically include an example script. I can build something that allows to include a file that is next to the index.md and it will be displayed as source code.

The current state is still very rough but can be seen here. It is fully generated. No manual work done yet. https://basiljs.github.io/

@b-g
Copy link
Member

b-g commented Feb 20, 2017

👍 + Great!

Just ... what is the difference between a collection and an array? Can/should we not simply always refer to array?

@fabianmoronzirfas Can you as the master of doc simply define the jsDoc style/structure/rules? Might be easier and quicker than to have a long thread on this.

@ff6347
Copy link
Member Author

ff6347 commented Feb 20, 2017

what is the difference between a collection and an array?

A Collection is an ID own thingy. You can call the everyItem and item and so on functions on it. They don't work on Arrays.

Can you as the master of doc simply define the jsDoc style/structure/rules? Might be easier and quicker than to have a long thread on this.

Yes I can! muahahahahahah!

@trych
Copy link
Contributor

trych commented Feb 20, 2017

The subpages need a lot of love. I can generate them but if we want to add further infos we need to write this by hand.

Yep, let's try to generate as much as possible, but also allow for additional info.

What we can do is automagically include an example script.

We should differentiate between snippets and example scripts. We should absolutely have a snippet for each function or even multiple if there are several options per function. A link to an example script would also be nice, but should only be an addition. It could be a link to a script sitting in the example scripts directory (which btw. we also wanted to move to a separate repository).

@ff6347
Copy link
Member Author

ff6347 commented Feb 20, 2017

We should differentiate between snippets and example scripts.

True

It could be a link to a script sitting in the example scripts directory (which btw. we also wanted to move to a separate repository).

Also True

@ffd8
Copy link
Member

ffd8 commented Feb 20, 2017

Hi guys– sorry for delay on this, got pulled into a webapp dev for our school since december and my basil.js love has suffered... nevertheless, great plans above! Sure- happy to join for splitting up the files and going through with fine comb.

Re: Subpages– there is one benefit to having basic text descriptions dumped like it is on the current website, allowing one to do a 'find' across the page for keywords (students use this all the time if not knowing the exact name of function). I wonder if there's a nice compromise that allows the organized reference list first (like we have it), descriptions (as we have it), then an accordion-style expanding snippet and potential isolated function example (when it exists). All staying within the same page, to avoid having to go forward/back/reload all the time? Just reveal/hide on demand.

@ff6347
Copy link
Member Author

ff6347 commented Feb 21, 2017

Sure- happy to join for splitting up the files and going through with fine comb.

Okay. I will create some issues for that and assign each of us some files to review. Before I will write down some hints what to look for and how a comment should be formatted so we get a good result.

Re: Subpages– there is one benefit to having basic text descriptions dumped like it is on the current website, allowing one to do a 'find' across the page for keywords (students use this all the time if not knowing the exact name of function).

Yes this is what I also do. I was thinking of adding lunar.js to the page to allow a full text search.

I wonder if there's a nice compromise that allows the organized reference list first (like we have it),

Yes I want to create that.

descriptions (as we have it),

This too.

then an accordion-style expanding snippet and potential isolated function example (when it exists). All staying within the same page, to avoid having to go forward/back/reload all the time? Just reveal/hide on demand.

I actually don't want to overload the site with JS features. The site should work without JS (except for the full text search of course) IMHO. Having all the Snippets in there would make the site very large for no script users.

Having the subpage where we repeat the content of the index and go into detail is exactly what Processing & P5.js are doing. I think these references work pretty well.

@ffd8
Copy link
Member

ffd8 commented Feb 21, 2017

The site should work without JS (except for the full text search of course) IMHO.

I'm a jerk and I stopped caring about no-script users (~ 0.25 – 2% of users). I'd imagine if someone's looking into basil.js, they're down with javascript. But true, for a no-script person, all the expanded divs would go on and on..

Yes this is what I also do. I was thinking of adding lunar.js to the page to allow a full text search.

Very nice looking lib and I guess it's main benefit is filtering out (hiding) content that didn't match, vs doing a browser window search/find and it jumping to the highlighted word? Hm and a normal window search probably doesn't find collapsed text, so that lib would be helpful if it went that route.

The other reference pages do work well, was just throwing out the idea if there's a benefit to a single page mighty reference, but maybe not.

@ff6347
Copy link
Member Author

ff6347 commented Feb 21, 2017

I'm a jerk and I stopped caring about no-script users (…)

I was too (jquery-bootstrapping-light-boxing my live away), but then I just recently came across this post ("Most of the web really sucks if you have a slow connection") by danluu and it made me think. If I would add something like a accordion reveal/hide mechanism I would not code it myself from scratch. This would mean to add in some library (bootstrap?). Then we start adding in jQuery and what not and have one of these bloated sites. So my aim is to have a site that looks good and works well without JS. 😉 It is actually not about the no script users or being a jerk. It's about having a good connection or not. When I look up a reference in 3G on the train or the subway (that happens more often to me than you actually think) I want it to load fast. I also have been in Quito recently. The slow and laggy connections in hotels made it hard to use any of the JS magic sites. Anyway. I could go on and on. I think you get my point.

The other reference pages do work well, was just throwing out the idea if there's a benefit to a single page mighty reference, but maybe not.

Sure. I get it. As you see I thought a lot about it too. The subpages would also allow easier to contribution of tutorials and snippets. Currently they are Markdown only. Contribute a snippet into a jekyll/liquid index.html is really hard if you don't know liquid.

@ff6347
Copy link
Member Author

ff6347 commented Feb 21, 2017

I wrote some remarks on how to use JSDoc comments in Basil. See the wiki. https://github.com/basiljs/basil.js/wiki/Writing-Documentation

Is something missing?

@ff6347
Copy link
Member Author

ff6347 commented Feb 21, 2017

One thing that is valid for all of us is using @return instead of @returns.

@b-g
Copy link
Member

b-g commented Feb 21, 2017

@fabianmoronzirfas Clear and good! Works for me, can't give more feedback before I started to document something!

@trych
Copy link
Contributor

trych commented Feb 21, 2017

Really cool!

Is something missing?

Yes, we still need to decide how to describe function arguments that can take different roles, depending on how you use the function. Example b.color(): The first property can either be a string (name of a color), a value for R, a value for C or a grey value. Again, I think we should have a look at how p5.js solves this. They list all possible combinations of arguments IIRC. I can check it in more detail later, I'm in a hurry right now.

@ff6347
Copy link
Member Author

ff6347 commented Feb 21, 2017

I would say like this?

/**
 * Creates a new RGB or CMYK color and adds the new color to the document, or gets a color by name from the document. The default color mode is RGB.
 *
 * @cat Color
 * @method color
 * @param  {String|Number} [a] Get color by name if you add a string or create grey colr by number.  
 * @param  {String|Number} [b] Depends on primary argument. If a is a Number and b is a String it is the name of the created color. If it is also a Number it is the intensity of Key added to a color.  
 * @param  {String|Number} [c] and so on
 * @param  {String|Number} [d] 
 * @param  {String|Number} [e] 
 * @return {Color} found or new color
 */
pub.color = function() {

A lot of it depends on the description. The function description should already contain how the values are used.

@trych
Copy link
Contributor

trych commented Mar 5, 2017

Some questions:

1

What type do I write down for a @param that holds a basil constant? Example:

/**
* @cat Document
* @subcat Primitives
* @method beginShape
* @param shapeMode Set b.CLOSE if the new Path should be auto-closed.
*/

To what should the shapeMode param be set? b.CLOSE itself is a basil constant, the parameter itself could either take this constant (which holds in effect a string) or be left blank.

So what do I write there?

2

Should we change the descriptions to always be in one line only?

3

Should I create separate PR's for A) making everything consistent and B) improve the descriptions themeselves? Or should this both be put in the same PR?

@ff6347
Copy link
Member Author

ff6347 commented Mar 6, 2017

About 1: I would say string…

/**
* @cat Document
* @subcat Primitives
* @method beginShape
* @param {String} shapeMode Set b.CLOSE if the new Path should be auto-closed.
*/

About 2:

I would say no. We can have multiline descriptions even with HTML to emphasize things.

About 3:

I would say make one PR for one "walkthrough" of a file. No need for to many PRs

@ff6347
Copy link
Member Author

ff6347 commented Mar 6, 2017

BTW we can have also code snippets directly in the code. See https://basiljs.github.io/#stories Looks pretty good.

@trych
Copy link
Contributor

trych commented Mar 6, 2017

4

From the Wiki:

Then follows the variable description. Start it with a capitalcase word and end it with a period.

Should this also be the case for return values?

5

What type do I write down for a @param that can take all sorts of pageItems (rect, polygons, textFrame and a lot more). Just a generic {PageItem} as it is the meta class of all specific page items?

@ff6347
Copy link
Member Author

ff6347 commented Mar 7, 2017

About 4: Return values have no variable name but a description. Should also be a sentence.

About 5: You can write {Rectangle|Polygon|TextFrame} or you just use the {PageItem} I would prefer the first.

@trych
Copy link
Contributor

trych commented Mar 7, 2017

You can write {Rectangle|Polygon|TextFrame} or you just use the {PageItem} I would prefer the first.

I just checked, technically it would result in a list of 47 different types of page items. I think I'll just use {PageItem} instead. ;)

@ff6347
Copy link
Member Author

ff6347 commented Mar 7, 2017

😬👌🏽

@trych trych mentioned this issue Mar 9, 2017
@ff6347
Copy link
Member Author

ff6347 commented Mar 14, 2017

I noted something else. Some of the optional values have in their description an Optional: Foo bah baz I would vote to remove this. If a parameter is optional it is marked with [name] square brackets around the parameter name. There is no need to write this again in the description. I can extract the information that something is optional and mark it as that.

The problem I see there is that if it is forgotten (see https://basiljs.github.io/#stories) and I don't mark them automatically we loose something. If we force to write in every description the Optional: like here https://basiljs.github.io/#paragraphs we have a doubled information. You get what I mean? Don't you?

@trych
Copy link
Contributor

trych commented Mar 14, 2017

I would vote to remove this.

Sure, let's take it out. I just checked the files that I was assigned to, they all seem fine as they are.

@ffd8
Copy link
Member

ffd8 commented Mar 20, 2017

Great docs progress + wiki! Starting to work on color doc and ran into this tricky bastard.. I started looking at Processing/P5js/processingjs – and noticed they all use a 'syntax' section instead of params in the function name. Our quasi-merged method seems fine except on ones like b.color() + b.fill(), due to the variations:

b.color("get_colorname");
b.color(GRAY, ["set_colorname"]);
b.color(R, G, B, ["set_colorname"]);
b.color(C, M, Y, K, ["set_colorname"]);

Continuing from your example above, it might be something like the following:

/**
 * Creates a new RGB / CMYK color and adds it to the document, or gets a color by name from the document. The default color mode is RGB.
 *
 * @cat Color
 * @method color
 * @param {String|Number} color Get color by name with a string or create gray color by number.  
 * @param {String|Number} [b] Set color name if first value is a number or set second value of new color.
 * @param {String|Number} [c] Set third value of new color.
 * @param {String|Number} [d] Set color name if first three values are numbers or set fourth value of new color.
 * @param  {String} [e] Set color name if first four values are numbers.
 * @return {Color} Found or new color
 */

– but that's still pretty abstract. Far from suggesting any major mod of docs per function, I'm wondering if the smoothest way to deal with this issue is leaving a basic param in the function like it was b.color(getColor, [setColor]) and then in the examples section, listing my variations above, commented out, so it's known to be non-runable code?

@trych
Copy link
Contributor

trych commented Mar 20, 2017

but that's still pretty abstract. Far from suggesting any major mod of docs per function, I'm wondering if the smoothest way to deal with this issue is leaving a basic param in the function like it was b.color(getColor, [setColor]) and then in the examples section, listing my variations above, commented out, so it's known to be non-runable code?

I like that suggestion and also agree, that describing each variation in the parameters themselves is too abstract and confusing. Showing the possible combinations of parameters is much more helpful.

The p5.js documentation does something similar and I find it very clear.

@ff6347
Copy link
Member Author

ff6347 commented Mar 21, 2017

and noticed they all use a 'syntax' section instead of params in the function name.
I don't get it.

but that's still pretty abstract.

Yes it is a bit abstract. I still would prefer having it the same way as the rest of the docs and not creating a exception. You can add examples to make it clearer.

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

No branches or pull requests

4 participants