-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: add JSDoc example #8533
base: master
Are you sure you want to change the base?
feat: add JSDoc example #8533
Conversation
Confirmed: linonetwo has already signed the Contributor License Agreement (see contributing.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @linonetwo
@@ -1,4 +1,5 @@ | |||
/*\ | |||
// @ts-check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It enables VSCode and ts cli regard this file as .ts
file. Maybe this enable different parse mode of VSCode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs seem to imply that one can also use a separate "jsconfig.json" file. Would that be possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try again and find VScode works with or without this. What make it work is open both file as tab...
I need to do more experiment.
@@ -1,4 +1,5 @@ | |||
/*\ | |||
// @ts-check | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to change the existing /*\
pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I find @module $:/core/modules/parsers/wikiparser/rules/codeblock.js
is useless for us, so the comment here don't need to be JSDoc anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -11,7 +12,28 @@ Wiki text rule for code blocks. For example: | |||
``` | |||
``` | |||
|
|||
@module $:/core/modules/parsers/wikiparser/rules/codeblock.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should extend the JS deserialiser to recognise @module
as a synonym for title:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not working, while this usage exists. I need to figure out why.
@linonetwo -- After your latest changes, does VSCode show useful type info in the tooltips now? |
@pmario Yes, I find I'm experiment with the tidddlywiki npm package, because this PR is intended for providing AST type for plugin developers. |
…/core/modules/...')`. Only works inside this project.
Solution to fix change var Widget = function(parseTreeNode,options) {
this.initialise(parseTreeNode,options);
}; to function Widget(parseTreeNode,options) {
this.initialise(parseTreeNode,options);
}; and add |
core/modules/wiki.js
Outdated
|
||
/** | ||
* @type {Widget} | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the 2 comment blocks be merged to 1 block, or do we need 2 of them?
* @param {string} title - The title of the tiddler. | ||
* @param {string} [defaultText] - The default text to return if the tiddler is missing. | ||
* @returns {string | null | undefined} - The text of the tiddler, undefined if the tiddler is missing, or null if the tiddler is being lazily loaded. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the type text should be as short as possible, but still understandable. Your definition contains "undefined", which is not returned. So I would suggest:
/**
* Returns the tiddler text-field. If field `_is_skinny` is present it triggers a "lazyLoad" event.
*
* @param {string} title - Tiddler title
* @param {string} [defaultText] - Returned if tiddler is missing
* @returns {string | null } - Returns the "text-field" or an empty string, "defaultText" if the tiddler is missing or null if the tiddler is being lazily loaded
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When add @returns
, it means it always return, so can't remove undefined
here. It is same in TypeScript.
* @property {number} attributes.language.start - The start position of the language string in the source text. | ||
* @property {number} attributes.language.end - The end position of the language string in the source text. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO having @property {string} type
and then - The type of the widget, which is "codeblock".
is the same thing. Both elements describe: "What it is". -- I think the second part should describe: "What it does", otherwise we can skip it.
- @Property does describe -- What it is
- Text describes -- What it does
eg:
/**
* Represents the parser `codeblock` rule.
*
* @typedef {Object} CodeblockNode
* @property {string} rule - Always "codeblock"
* @property {string} type - Always "codeblock"
* @property {number} start - Rule start marker in source text
* @property {number} end - Rule end marker in source text
* @property {Object} attributes - Contains "code" and "language" objects
* @property {Object} attributes.code - The code attribute object
* @property {string} attributes.code.type - Always "string"
* @property {string} attributes.code.value - Actual code
* @property {number} attributes.code.start - Start position of code in source text
* @property {number} attributes.code.end - End position of code in source text
* @property {Object} attributes.language - The language attribute object
* @property {string} attributes.language.type - Always "string"
* @property {string} attributes.language.value - Language specified after the triple backticks, if any
* @property {number} attributes.language.start - Start position of the string in source text
* @property {number} attributes.language.end - End position of the string in source text
*/
Should be as short as possible and still valid. Especially see type, rule, start, end
which are properties of "CodeblockNode"
I did remove the "full stops" from all @
elements. They are not needed
The first intro sentence has a full stop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'd copy that.
I have to admit, all jsdoc are generated by github copilot. I don't have time for this detail, I will leave it for future refinement. I'm still debugging the type when I'm using it in my plugin project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit, all jsdoc are generated by github copilot. I don't have time for this detail, I will leave it for future refinement. I'm still debugging the type when I'm using it in my plugin project.
The new type information will significantly increase the TW code-size. So if there is redundant information it should be removed.
And even more important it has to be valid. So if the co-pilot only adds comments in a more verbose form than the parameters are. There should not be any comments at all -- They have no value.
So if there are no comments, we actually know, that we have to add them manually. IMO for the tooltips it would be OK to start with the type info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will significantly increase the TW code-size
we must remove comments before publishing HTML wiki. I think these JSDoc will basically double the size.
it has to be valid
This won't be a big problem, because 1. I use the method body as input too. And 2. we can auto merge "comment" type of PR based on #7542 , so people can update comment quickly. I find this is quite frequent when maintaining https://github.com/tiddly-gittly/TW5-Typed
…et'. An explicit type annotation may unblock declaration emit.
Plugin developers can install typescript globally, and run But I'm still working on this, some imported type is not working currently. |
* @param {boolean} [options.parseAsInline=false] - If true, the text will be parsed as an inline run. | ||
* @param {Object} options.wiki - Reference to the wiki to use. | ||
* @param {string} [options._canonical_uri] - Optional URI of the content if the text is missing or empty. | ||
* @param {boolean} [options.configTrimWhiteSpace=false] - If true, trims white space according to configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the full-stop at the end of the comment text can be removed.
IMO "The" and "the" can be removed -- most of the time. So it does not obscure the important content. eg:
* @param {string} type - Content type of the text to be parsed
* @param {string} text - Text to be parsed
* @param {Object} options - Parser options
* @param {boolean} [options.parseAsInline=false] - If true, the text will be parsed as an inline run
* @param {Object} options.wiki - Reference to the wiki store in use
* @param {string} [options._canonical_uri] - Optional URI of the content if text is missing or empty
* @param {boolean} [options.configTrimWhiteSpace=false] - If true, the parser trims white space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this will be the updated prompt:
add jsdoc, only output jsdoc for this class and its method, no impl. full-stop at the end of the comment text can be removed. "The" and "the" can be removed -- most of the time. So it does not obscure the important content.
Now plugin developers can get type hint directly generated from TW source code: 1. tsconfig.json in plugin rootAs a plugin developer, in your project: "compilerOptions": {
"paths": {
// Allow `import('$:/core/modules/...')` instead of `import('tiddlywiki/core/modules/...')`. Only works inside this project.
"$:/core/*": ["node_modules/tiddlywiki/types/core/*"],
"tiddlywiki/*": ["node_modules/tiddlywiki/types/*"],
}, Don't need 2. run
|
Just an example yet.