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

Morebits: add documentation #481

Merged
merged 5 commits into from
Jan 12, 2019

Conversation

siddharthvp
Copy link
Member

Added some documentation (jsdocs) for several of morebits classes. Since jsdocs are shown by code editors in tooltips, this would greatly help the next generation of script writers.

I converted existing documentation comments to jsdoc-comments for wiki.page, wiki.api, wiki.preview, string, array, regexp classes while adding my own $0.02 here and there. For wikitext.page, it is written from scratch.

In the 3rd commit, I rearranged the order of several functions in wiki.page, hence the diff for that class may look quite messy.

@siddharthvp
Copy link
Member Author

Hey can someone merge this please! This is just a documentation upgrade. Nothing to be tested. I feel that this will help new developers get acquainted with the twinkle codebase more easily.

Copy link
Collaborator

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, mostly minor nitpicks ;) Yes we should probably add more linter rules of some of this, especially extraneous spacing.

Also, since you're adding JSDoc blocks, why not specify @returns (assuming it returns something)?

morebits.js Outdated Show resolved Hide resolved
morebits.js Outdated
/**
* Gives an array of substrings of `str` starting with `start` and
* ending with `end`, which is not in `skiplist`
* @param {string} str @param {string} start @param {string} end @param {(array|string)} [skiplist]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this valid JSDoc? Either way, I'd argue having each param on a newline is easier on the eyes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the description makes it clear the functionality of each parameter, I thought I'd rather not waste space putting each on a new line.

Fixed now.

morebits.js Outdated Show resolved Hide resolved
morebits.js Outdated Show resolved Hide resolved
morebits.js Outdated Show resolved Hide resolved
morebits.js Outdated Show resolved Hide resolved
};

/**
* `pageText` - string containing the updated page text that will be saved when save() is called
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use JSDoc syntax here? (and elsewhere below)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the odd behaviour it causes (in my editor, VScode, at least): putting @param makes the tooltip unnecessarily carry the parameter's description twice. Once at the top: just below the function prototype and once again in the parameters' section. This is especially poor-looking for those large param descriptions like in this.setMinorEdit().

Make it jsdocs if you feel like it.

morebits.js Outdated Show resolved Hide resolved
@@ -2847,7 +2940,7 @@ Morebits.wiki.page = function(pageName, currentAction) {
};
}; // end Morebits.wiki.page

/** Morebits.wiki.page TODO: (XXX)
/* Morebits.wiki.page TODO: (XXX)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be

/**
 * Morebits.wiki.page TODO: (XXX)
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because in that case, it gets treated as part of the doc comment for Morebits.wiki.preview (which is the next function after this).

@MusikAnimal
Copy link
Collaborator

Thanks for this. I'd love to see all of Twinkle have propoer JSDocs. This is a great start.

@MusikAnimal MusikAnimal merged commit c3c32dc into wikimedia-gadgets:master Jan 12, 2019
siddharthvp added a commit to siddharthvp/twinkle that referenced this pull request Jan 19, 2019
* morebits: improve documentation of wiki.api, string, array classes

* morebits: wiki.page: generate jsdoc comments on basis of existing doccomment

* morebits: wiki.page: prune the doc comment at the top to show just the highlights, rearranged functions to a more logical order, to roughly match with the order in the original doc comment

* morebits: use jsdocs on wiki.preview; minor changes to wiki.api

* morebits: add jsdocs for wikitext.page + minor changes elsewhere
siddharthvp added a commit to siddharthvp/twinkle that referenced this pull request Jan 19, 2019
* morebits: improve documentation of wiki.api, string, array classes

* morebits: wiki.page: generate jsdoc comments on basis of existing doccomment

* morebits: wiki.page: prune the doc comment at the top to show just the highlights, rearranged functions to a more logical order, to roughly match with the order in the original doc comment

* morebits: use jsdocs on wiki.preview; minor changes to wiki.api

* morebits: add jsdocs for wikitext.page + minor changes elsewhere
Amorymeltzer added a commit to Amorymeltzer/twinkle that referenced this pull request Jan 22, 2019
siddharthvp added a commit to siddharthvp/twinkle that referenced this pull request Jan 23, 2019
Amorymeltzer added a commit that referenced this pull request Jan 23, 2019
morebits: add back function accidentally removed in #481
@siddharthvp siddharthvp deleted the morebits branch October 22, 2020 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants