-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
jsdoc contradiction to code in GameEntity #43
Comments
I think I would prefer to add the missing
Many thanks for this! According to my experience with |
See #44
I had a quick glance at the README over there and might give it a shot. Another possibility would be to convert the code base entirely to typescript and recompile it to es6 but I guess you would not want that. While doing the conversion, there were some discrepancies I noticed (sadly I forgot to write them down as I was in a hurry), would you prefer to receive separate tickets on each of these? |
I guess I have to start using tags now 😅 .
No, sorry.
I think you can share them right here in this issue. |
There is now a tag for the new |
Ok, here they come:
export interface Triangle {
/**
* The first vertex position.
*/
a: Vector3;
/**
* The second vertex position.
*/
b: Vector3;
/**
* The third vertex position.
*/
c: Vector3;
} Where in
These are the ones that I could reconstruct. |
Types added as DefinitelyTyped/DefinitelyTyped#52724 |
I have quickly fixed point 2, 3 and 4 (8fdd2a9). However, I am not going to create a separate class for the inline type The JSDoc for nullable properties would look like so, right? * @type {HalfEdge | null} |
You don't have to, it should be sufficient to add the following anywhere in the same file. /**
* A triangle shape used in {@link Ray#intersectTriangle}.
*
* @typedef {Object} Triangle
* @property {Vector3} a The first vertex position.
* @property {Vector3} b The second vertex position.
* @property {Vector3} c The third vertex position.
*/
Yes, this is correct. However, I think some of the properties should become mandatory. |
In |
Nice! Did not know this was possible. 3d0aefc |
The issue is still open and I fail to see a definitive answer there, as Don McCurdy (intentionally not linked here, did not want to cause a notification) pointed out, a factory is needed to create the instances properly.
Larger bundles were not my main concern but rather the non strict types for properties as mentioned above. Another solution would be, to add static |
I've investigated the nullable issue a bit more. Instead of adding
Do you see any issue with that approach? If not, I would like to adapt it since the JSDoc generator will create a nice |
I don't see any problem with that as it appears that in jsdoc |
Okay, all nullable properties should now be documented correctly 👍 . 2e2d1bd |
For the record, the types have just been published as: @types/yuka. |
@discordier How does it work? Do the types get compiled from the source JSDocs? Or maintaining both by hand? |
I just created typescript definitions for all types in yuka (I wonder if I should submit them as PR here) and found that we have a contradiction in return types in
GameEntity.js
yuka/src/core/GameEntity.js
Lines 202 to 216 in 6314fd6
See, we define that we are returning
this
, while we are in fact a void method.I wonder what the real intention is here, as I personally do not see any benefit in chaining either method (To be honest, most methods in GameEntity won't be chained in real life applications).
Shall we now add the missing
return this
or update the jsdoc to reflect@return void
?The text was updated successfully, but these errors were encountered: