Page MenuHomeWildfire Games

Consider Google Closure Compiler type expressions in JSdoc in Health.js (rP22196)
ClosedPublic

Authored by Stan on Apr 25 2019, 1:52 PM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22229: Fix missing spaces and non compliant code with regards to the coding…
Summary
Test Plan

Test it matches the CC.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Stan created this revision.Apr 25 2019, 1:52 PM
Stan edited the summary of this revision. (Show Details)
Stan edited the summary of this revision. (Show Details)

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Health.js
| 251| 251| 	}
| 252| 252| 
| 253| 253| 	Engine.DestroyEntity(this.entity);
| 254|    |-}
|    | 254|+};
| 255| 255| 
| 256| 256| Health.prototype.Increase = function(amount)
| 257| 257| {

binaries/data/mods/public/simulation/components/Health.js
| 254| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1276/display/redirect

Stan updated this revision to Diff 7845.Apr 25 2019, 2:01 PM

Add missing semicolon to fix Vulkan's warning

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1277/display/redirect

If we have more than one revision that is called "Fix <number>" then noone will know which one is which.
So always name them according to their content.

if the convention says so I will abide.

Notice that abiding the conventions is not an end in itself, but to prevent confronting the reader with a different syntax wherever they look.
So abiding by the conventions is coincidence, what we're trying to achieve is to make the codebase as comfortable to the reader.

https://stackoverflow.com/questions/28763257/jsdoc-return-object-structure

Following specifications means that one is as close as possible to the source.
Google and Stackoverflow can help one find the primary source, but following them without looking at the exact definitions is what has led to the Google style use.

So I can confirm from the JSdoc sources that the stackoverflow recommendation seems to be the most recent JSdoc way of doing this:
http://usejsdoc.org/tags-returns.html
https://github.com/jsdoc3/jsdoc/issues/1045
https://github.com/jsdoc3/jsdoc/issues/931
https://github.com/jsdoc3/jsdoc3.github.com/issues/115

But the question is whether the comments need to be so verbose to begin with.
If we do this in all of the codebase, then our codebase might swallow up drastically for no benefit to the reader of the code, only benefit to the reader of the comments that does not look at the code, and that's not really a use case is it?

You're lucky, this time I found the Philip quote in 2015-08-31-QuakeNet-#0ad-dev.log:

23:32 * Philip` thinks it's entirely non-useful to read Doxygen output, except for code that's specifically designed to be a public API
23:33 < Philip`> since if it's not a public API, you're only going to care about it if you're working on that code or something closely interacting with it, and in that case you should read the actual code
23:35 < Philip`> (It's still useful to use Doxygen syntax for documenting that code, for the same reasons that it's useful to have coding conventions for writing the code (i.e. it eliminates a load of unnecessary variability), but the contents of the documentation should assume the reader is reading and understanding the code too)

/**
 * @typedef {Object} HealthStatusChange
 * @property {boolean} killed Whether the entity was killed.
 * @property {number} change The amount of health was removed before reaching 0.
 * Reduces entity's health by amount HP. Kill if required.
 * @param {number} amount the amount of HP to be substracted from the hitpoints.
 * @return {HealthStatusChange} the status change of the entity.

Why not just

/*
 * Reduces entity's health by the given amount and kill it if no HP are left.
 * @returns the amount of HP lost and whether the entity was killed.
 */
binaries/data/mods/public/simulation/components/Health.js
180 ↗(On Diff #7845)

(Isn't the Reduces comment relating to the change property this way?)

Stan retitled this revision from Fix rP22196 to Fix rP22196 Coding Convention breach.Apr 25 2019, 2:56 PM
Stan added a comment.Apr 25 2019, 3:10 PM
In D1848#76171, @elexis wrote:

If we have more than one revision that is called "Fix <number>" then noone will know which one is which.
So always name them according to their content.

Done.

Notice that abiding the conventions is not an end in itself, but to prevent confronting the reader with a different syntax wherever they look.
So abiding by the conventions is coincidence, what we're trying to achieve is to make the codebase as comfortable to the reader.

It is an end to me. It's how I get my patches committed, and add new features to the game. I don't really care as long as the player or the modder benefits from it.
Sure, but the syntax was similar enough that wraitii thought it was JSDoc.

But the question is whether the comments need to be so verbose to begin with.
If we do this in all of the codebase, then our codebase might swallow up drastically for no benefit to the reader of the code, only benefit to the reader of the comments that does not look at the code, and that's not really a use case is it?
You're lucky, this time I found the Philip quote in 2015-08-31-QuakeNet-#0ad-dev.log:

23:32 * Philip` thinks it's entirely non-useful to read Doxygen output, except for code that's specifically designed to be a public API
23:33 < Philip`> since if it's not a public API, you're only going to care about it if you're working on that code or something closely interacting with it, and in that case you should read the actual code
23:35 < Philip`> (It's still useful to use Doxygen syntax for documenting that code, for the same reasons that it's useful to have coding conventions for writing the code (i.e. it eliminates a load of unnecessary variability), but the contents of the documentation should assume the reader is reading and understanding the code too)

[...]

Why not just

[...]

I respect people choice to not add any comments, nor documentation because it clutters their view of the code and the comments might always been outdated. According to what I understand from Philip we shouldn't have doxygen at all because everything is a private API. Other software don't interact with ours. That's his view not mine.

My view is that getter and setters have (or rather should) have an explicit enough name making doxygen useless.

But

  1. The goal of this doxygen block is to allow me to see what the function returns, and what members of the object I will need. If I see that function called somewhere, I can just look at the top, and see what I need to call
  2. Since there are no explicit types in JavaScript, it allows me to make sure I'm always returning the same kind of stuff because I hate that you can return an object, a string and an array from the same function.
  3. Were I to create a new message for instance, I could make sure it shares the same components than the other messages, and there for have similar treatments instead of having a completely different handling code
  4. At some point it would be nice to generate actual documentatio from the code see https://wildfiregames.com/forum/index.php?/topic/20728-jsdoc-documentation/ Would suck having to add stuff because it's incomplete
  5. Didn't need to be verbose if we used the google syntax.
binaries/data/mods/public/simulation/components/Health.js
180 ↗(On Diff #7845)

Yes it is.

Stan retitled this revision from Fix rP22196 Coding Convention breach to Fix rP22196 Health.js Coding Convention breach.Apr 25 2019, 3:11 PM

(Long discussion because it's a Coding Convention change in question)

Done.

Thanks

Notice that abiding the conventions is not an end in itself, but to prevent confronting the reader with a different syntax wherever they look.
So abiding by the conventions is coincidence, what we're trying to achieve is to make the codebase as comfortable to the reader.

It is an end to me. It's how I get my patches committed, and add new features to the game. I don't really care as long as the player or the modder benefits from it.

If we don't know the purpose of a policy then we don't know why the policy exists and whether it's effective, efficient, in need of change or improveable.
If we know about the purposes of the policy, then the policy doesn't matter much anymore because we follow it implicitly when furthering the purposes of the policy.
People should be motivated by the benefits of the educated choices, not the legal necessity for educated choices.
Wildfire Games is the creator of the policy (coding convention), so they have the freedom to investigate or revise the policy to achieve the purposes.
First come the purposes of the coding convention, the convention is deduced from that.

If the convention is wrong, we may not follow it but have to change it.

If we come to the conclusion that Google syntax or some alternative syntax is better, we may consider to put that into Coding_Conventions.
Such a decision ought to be based on the what we want to achieve, i.e. a consideration of the clients that we want to support officially.
The results could go to the wiki page as well.
(Hypothetically... depending on what we want to achieve...)

I respect people choice to not add any comments, nor documentation because it clutters their view of the code and the comments might always been outdated. According to what I understand from Philip we shouldn't have doxygen at all because everything is a private API. Other software don't interact with ours. That's his view not mine.

No:

23:35 < Philip`> (It's still useful to use Doxygen syntax for documenting that code, for the same reasons that it's useful to have coding conventions for writing the code (i.e. it eliminates a load of unnecessary variability), but the contents of the documentation should assume the reader is reading and understanding the code too)

That's also been my conclusion after we abandoned the idea to generate those HTML JSdoc pages and after I stopped adding many @params to the JS GUI, must have been end of 2015 (must have been august, because the discussion with Philips was related to that topic).
The principle is:
If you document, then document in a defined syntax.
But don't document something just to use the defined syntax.

The reason is

23:33 < Philip`> since if it's not a public API, you're only going to care about it if you're working on that code or something closely interacting with it, and in that case you should read the actual code

If we do this in all of the codebase, then our codebase might swallow up drastically for no benefit to the reader of the code, only benefit to the reader of the comments that does not look at the code, and that's not really a use case is it?

In the current example those are only 2-4 lines, but if we make this a commonpractice, it would be several hundred lines.

I in particular didn't say you shouldn't add anything, but gave the example:

/*
 * Reduces entity's health by the given amount and kill it if no HP are left.
 * @returns the amount of HP lost and whether the entity was killed.
 */

I'm not stating this is better period, but I would like to have the reasons expressed and analyzed as to why one of the two is better.
Perhaps Philip and me are missing some valid use case.
Just filling in all fields of the tooltips of the clients GUI to me seems like lots of maintenance work without benefit, for both the people who need to write the partially redundant tooltips, but also to everyone who reads them to spot the non-redundant information.

We should actually document stuff in JSdoc that isn't obvious from reading few lines of code (on a per case basis), and only that.
(Exception is the API case, for example the rmgen library was sometimes designed as such.)

Mostly I think that I already learn all the relevant information from one of these lines

return { "killed": false, "change": 0 };

Instead of going through 5 lines of comments

* @typedef {Object} HealthStatusChange
* @property {boolean} killed Whether the entity was killed.
* @property {number} change The amount of health was removed before reaching 0.
* Reduces entity's health by amount HP. Kill if required.
* @param {number} amount the amount of HP to be substracted from the hitpoints.
* @return {HealthStatusChange} the status change of the entity.

Adding comments also always comes with the cost that their correctness comes under scrutiny.
So basically the only ones reading these comments are the ones who already learnt the information from the code and now only have the disadvantage left that there is more text to scroll out of view or more text to question for correctness.

Another short alternative would be:

/*
 * Reduces entity's health by the given amount and kill it if no HP are left.
 * @returns for example { "amount": -100, "killed": true }
 */

It's not JSdoc, but it's JS and the JSdoc alternative is also ugly (typedef), and the other alternative is Google syntax.
It's also redundant with the clearly visible return statements here.

* Reduces entity's health by amount HP. Kill if required.
* @return {{ "killed": boolean, "change": Number }} the status change of the entity.

So I guess, Google syntax might actually better in this case. Not to display tooltips but to express the information that we want to express but cannot express in JSdoc without long workarounds.
But whitelisting Google syntax might mean a lot of proper JSdoc might be rewritten for no reason other than syntax management, and the code comment format would become more heterogenous.

So I can only conclude that there is not enough evidence shown to me yet to use a second syntax, and I didn't see sufficient evidence that shows the benefit of the longer comment.

So I uphold the Google syntax concern, but I don't raise a concern if you commit the long JSdoc comment, although I see more benefit in the shorter JSdoc comment.
But more evidence might change my view.

The goal of this doxygen block is to allow me to see what the function returns, and what members of the object I will need. If I see that function called somewhere, I can just look at the top, and see what I need to call

  • But in order to use the function you need to inevitably learn all the details of the functions, which means that you have as much code comments as you have code.
  • If you are not familiar with the 0 A.D. function, you need to become familiar with it first, regardless of whether you had seen the @params already.

I suppose you mean https://en.wikipedia.org/wiki/Information_hiding but I'm not sure that applies, because you actually need the information of the function that you use.

Since there are no explicit types in JavaScript, it allows me to make sure I'm always returning the same kind of stuff because I hate that you can return an object, a string and an array from the same function.

There are only explicit types in JavaScript, but they are per value, not per variable.
I agree that a function should only return at most one or two types and rely on Falsy/Truthy mechanics to simplify.
But relying on the code instead of a code comment means less likelihood to do a mistake. The code can't lie, the comments can.

Were I to create a new message for instance, I could make sure it shares the same components than the other messages, and there for have similar treatments instead of having a completely different handling code

At some point it would be nice to generate actual documentatio from the code see https://wildfiregames.com/forum/index.php?/topic/20728-jsdoc-documentation/ Would suck having to add stuff because it's incomplete

That's exactly where the Philip discussions came into play, either directly that discussion or that timeperiod.
The JSdoc HTML pages are not of much use in comparison to reading the annotated code, unless it's some API.

binaries/data/mods/public/simulation/components/Health.js
180 ↗(On Diff #7845)

I mean does the JSdoc client display that description as a property description or as the function description?
I thought the function description was supposed to be in the first line.
So perhaps that was only a silent convention.

215 ↗(On Diff #7845)

unneeded parenthesis

elexis retitled this revision from Fix rP22196 Health.js Coding Convention breach to Consider Google JSdoc syntax in Health.js (rP22196).Apr 25 2019, 5:30 PM
Stan updated this revision to Diff 7848.Apr 25 2019, 5:48 PM
  • Remove unneeded parenthesis.
Stan marked 3 inline comments as done.Apr 25 2019, 5:49 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Health.js
180 ↗(On Diff #7845)

I meant that description was redundant. The description was indeed displayed as the function's description.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1280/display/redirect

Stan marked an inline comment as done.Apr 25 2019, 6:22 PM

If the convention is wrong, we may not follow it but have to change it.

What is a "wrong" convention ? Aren't all conventions inherently wrong for someone else ? They rely on things like common sense, which changes depending on the country, the education, the religion... etc.

If we come to the conclusion that Google syntax or some alternative syntax is better, we may consider to put that into Coding_Conventions.
Such a decision ought to be based on the what we want to achieve, i.e. a consideration of the clients that we want to support officially.
The results could go to the wiki page as well.
(Hypothetically... depending on what we want to achieve...)

Of course we should try to improve the existing conventions. Like should we use variable = undefined, or delete variable ?
About what we want to achieve I'm not sure. We are currently not trying to have the best documentation possible. We will probably never be at the level of the Boost library. I just like the Google Convention because it's easy for me to remember, it holds on one line, and most importantly it tells me what I should expect from the function.

If you document, then document in a defined syntax.

I have no issue with that.

But don't document something just to use the defined syntax.

I don't document just to use a syntax, I document because I use the documentation. I'm not sure I'm making sense here.

I'm not stating this is better period, but I would like to have the reasons expressed and analyzed as to why one of the two is better.
Perhaps Philip and me are missing some valid use case.
Just filling in all fields of the tooltips of the clients GUI to me seems like lots of maintenance work without benefit, for both the people who need to write the partially redundant tooltips, but also to everyone who reads them to spot the non-redundant > > information.

I don't know anything about the GUI, so I clearly have no idea on what should be done there. But from what I understood its mostly because it's a mess that we have an issue.

Mostly I think that I already learn all the relevant information from one of these lines

Yeah that works as well. I just assumed using a known syntax would be better than having it as text.

So I guess, Google syntax might actually better in this case. Not to display tooltips but to express the information that we want to express but cannot express in JSdoc without long workarounds.
But whitelisting Google syntax might mean a lot of proper JSdoc might be rewritten for no reason other than syntax management, and the code comment format would become more heterogenous.

Well we didn't edit every single cpp file to replace C casts by CPP casts nor did we edit all those files to replace NULL by nullptr.

/**
 * @return {Object} This is correct. 
 */

So I uphold the Google syntax concern, but I don't raise a concern if you commit the long JSdoc comment, although I see more benefit in the shorter JSdoc comment.
But more evidence might change my view.

From doing a quick search in the public folder, we mostly return stuff that are not objects or Object sometimes we don't give the type, sometimes we give multiple, sometimes there are '-' before the text sometimes not

We use @return 129 times in the public mod. 12 times @returns {object}/{Object} in 5 files
4 times array in 4 files. One time it could be {string[]} the rest is {Object[]}

One could also try to generate JSDoc and see if it recognizes google syntax. As a side note I wonder if we could use https://developers.google.com/closure/compiler/

But in order to use the function you need to inevitably learn all the details of the functions, which means that you have as much code comments as you have code.
If you are not familiar with the 0 A.D. function, you need to become familiar with it first, regardless of whether you had seen the @params already.
I suppose you mean https://en.wikipedia.org/wiki/Information_hiding but I'm not sure that applies, because you actually need the information of the function that you use.

Well I'd say it depends. If you want to ask the pathfinder to compute a path for you, you might not need to understand the specifics of the function, just the params and the return type will be enough. Or if you'd like to garrison a unit, only knowing that it will return true if it succeeds and that you need to pass the entity_id_t to the function is enough. No need to understand everything. Sure that you'll get to know everything better after a while because you'll probably end up reading the function for god knows what reason, but it isn't a requirement to me.

There are only explicit types in JavaScript, but they are per value, not per variable.
I agree that a function should only return at most one or two types and rely on Falsy/Truthy mechanics to simplify.
But relying on the code instead of a code comment means less likelihood to do a mistake. The code can't lie, the comments can.

Sure, but then the person who committed the code did a mistake because he didn't update the doc. That's also IMHO one of the reasons we do reviews. This whole thing is not about the code being incorrect, because code that doesn't match the CC can totally be correct.

That's exactly where the Philip discussions came into play, either directly that discussion or that timeperiod.
The JSdoc HTML pages are not of much use in comparison to reading the annotated code, unless it's some API.

That's right. However some people might just want to browse the website before delving in the code. We do have a doxygen page on our wiki IIRC.

  • Kills the entity if required is implied in Whether the entity was killed
  • The amount of health was removed before reaching 0, bit odd because the "before" is temporal. Maybe just The amount of health that was reduced?

Still unhappy about typedef as a pattern, since that will make for a custom JSDoc variable name such as HealthStatusChange for every single function that returns an object.

If we do a custom thing in every function, then we might just as well define this one Google syntax rule as an exception in our CC, and clearly state in that we prefer JSdoc when there is a choice.
(I.e. open to switching google syntax if we find persuading evidence, but not without further investigation.)

Actually it looks like JSdoc 3.2 already supports Google Closure Compiler.
http://usejsdoc.org/tags-type.html
http://usejsdoc.org/tags-param.html

Just not for return.

http://usejsdoc.org/tags-param.html provides conflicting formats for optional arguments:

An optional parameter (using JSDoc syntax)

  • @param {string} [somebody] - Somebody's name.

An optional parameter (using Google Closure Compiler syntax)
@param {string=} somebody - Somebody's name.

https://github.com/jsdoc3/jsdoc/issues/1045
states that @return {{x:Number, y:Number}} is actually supported and states that the documentation should be updated.

So in fact you have the reasons to keep the @return, it doesn't break the CC, the JSdoc documentation is wrong and the bug is reported upstream.

So future JSdoc approach would be to avoid the Google Closure Compiler syntax unless necessary to express the information that is intended to be expressed.
This means you could keep Google Closure Compiler syntax here, refrain from using it unless the where the Google Closure Compiler free syntax is somehow nicer, thus still getting a codebase that is homogeneously conforming to JSdoc and minimizing format arbitration.

(Thanks for your attention and sorry for your loss of time, but I hope it helps minimizing format arbitration in the future, this wasn't the first incidence of novel JSdoc style)

elexis retitled this revision from Consider Google JSdoc syntax in Health.js (rP22196) to Consider Google Closure Compiler type expressions in JSdoc in Health.js (rP22196).Apr 25 2019, 6:45 PM
Stan updated this revision to Diff 7853.Apr 25 2019, 6:54 PM
  • Add dashes
  • Readd the anonymous return object.
Stan added a comment.Apr 25 2019, 6:55 PM

@elexis Thanks for the detailed discussion and looking it up. Appreciated.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1282/display/redirect

So the outer brace comes from the @return defined here https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#return-type-description
The inner brace denotes an object, defined here https://github.com/google/closure-compiler/wiki/A-word-about-the-type-Object

It's a "record" type:
https://github.com/google/closure-compiler/wiki/A-word-about-the-type-Object#records

Haven't seen anything rules to whitespace after {, before }, and after colon :, all examples don't have it, only one after the comma. (We have it for JS objects, but this line is not intending to express a JS object, but to describe one in JSdoc.)
Also there are no quotes.
-> @return {{killed:boolean, change:number}}

(I guess one could read or run the code if one wanted to know, and I suspect clients don't break if they get a space after colon)

(I'm not sure if we want to reach this level of description, but it's concise:)

/** @param {{required:string, optional:(string|undefined)}} props */

(And reading code still seems more reasonable to me than expressing all of that in comments, since the code contains all information and all information contained in the code should be relevant or already hidden behind helper functions. So avoid novel syntax where you can.)

Stan added a comment.Apr 25 2019, 7:27 PM

Anything else we should fix ?

  • @return {{ "killed": boolean, "change": number }} - the status change of the entity.
  • @return {{ killed:boolean, change:number }} - number of health points lost and whether the entity was killed

Actually not sure why this speaks of killed, Aliveand HandleDeath because structures have health but are not alive nor dead.
But I'm satisfied now (as in exhausted interest)

Thanks for the fixes.

Stan updated this revision to Diff 7857.Apr 25 2019, 7:54 PM

Update comment

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1285/display/redirect

(See remarks about quotes and spaces)

Stan updated this revision to Diff 7860.Apr 25 2019, 9:43 PM

Arf, yes

Remove spaces and quotes

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1287/display/redirect

elexis accepted this revision.Apr 25 2019, 9:54 PM

The ones after {{ and before }} also, at least judging by the examples.

Thanks for the discourse, we learnt something for the future.

This revision is now accepted and ready to land.Apr 25 2019, 9:54 PM
This revision was automatically updated to reflect the committed changes.