Conversation
blueskyson
left a comment
blueskyson
left a comment
There was a problem hiding this comment.
Wow, I wonder why comment enhancements are left so long unmerged.
| /** | ||
| * Fetch a page from cache using preferred language and preferred platform. | ||
| * @param page {} A | ||
| * @returns {Promise<unknown>} |
There was a problem hiding this comment.
What is the difference between Promise<any> and Promise<unknown> ?
There was a problem hiding this comment.
These are both reserved keywords in tsc. I'm not sure if other type checking users them.
anymeans to disable type checking outright for the result.unknownmeans keeps type checking enabled, but enforces that the consumer of the Promise must be very defensive if they are to access any properties in the result.
In practice, this means that if the return is Promise<any> they caller can just make any wild assumption about what the result is without any complaints from the type checker. Meanwhile, with Promise<unknown> the caller will have to verify that properties exist before it can access them, because the type is documented as unknown at runtime.
| // Return all commands for a given platform. | ||
| // P.S. - The platform 'common' is always included. |
There was a problem hiding this comment.
Should we remove this then? Seems repeated.
| } | ||
|
|
||
| /** | ||
| * Print pages for a given platform.. |
There was a problem hiding this comment.
Multiple periods at the end of the sentence.
|
|
||
| /** | ||
| * Print a command page. | ||
| * @param commands {string[]} A given command to be printed. |
There was a problem hiding this comment.
"A given command" is incorrect when we are passing an array. Let's change this to plural.
|
|
||
| /** | ||
| * Update the cache. | ||
| * @returns {Promise<any>} A promise with the index. |
There was a problem hiding this comment.
Should this be "A promise with the cache"?
vitorhcl
left a comment
vitorhcl
left a comment
There was a problem hiding this comment.
LGTM after we address the left concerns.
However, we can resolve just some of them and merge it, this PR adds documentation for quite a lot of functions. It's better to have a not ideal documentation than almost no documentation.
| } | ||
|
|
||
| /** | ||
| * Print pages for a given platform.. |
There was a problem hiding this comment.
| * Print pages for a given platform.. | |
| * Print pages for a given platform. |
|
|
||
| /** | ||
| * Print a command page. | ||
| * @param commands {string[]} A given command to be printed. |
There was a problem hiding this comment.
| * @param commands {string[]} A given command to be printed. | |
| * @param commands {string[]} Given commands to be printed. |
| * Return all commands for a given platform. | ||
| * | ||
| * The 'common' platform is always included. | ||
| * @param platform {string} The platform. |
There was a problem hiding this comment.
| * @param platform {string} The platform. | |
| * @param platform {string} The desired platform. |
| } | ||
|
|
||
| /** | ||
| * Print a random page. |
There was a problem hiding this comment.
| * Print a random page. | |
| * Print a random page example. |
| } | ||
|
|
||
| /** | ||
| * Fetch stats from the cache folder. |
There was a problem hiding this comment.
| * Fetch stats from the cache folder. | |
| * Fetch stats from the cache folder for getting its last modified time (mtime). |
| // There is no need to re-read the index file again. | ||
| /** | ||
| * Check if a page is in the index. | ||
| * @returns {boolean} A boolean that indicates if the page is present. |
There was a problem hiding this comment.
| * @returns {boolean} A boolean that indicates if the page is present. | |
| * @returns {boolean} The presence of the page in the index. |
| } | ||
|
|
||
| /** | ||
| * Update the cache folder and returns the English entries. |
There was a problem hiding this comment.
| * Update the cache folder and returns the English entries. | |
| * Update the cache folder using a temporary directory, update the index and return it. |
|
|
||
| /** | ||
| * Print pages for a given platform.. | ||
| * @param singleColumn {boolean} A boolean to print one command per line. |
There was a problem hiding this comment.
| * @param singleColumn {boolean} A boolean to print one command per line. | |
| * @param singleColumn {boolean} Print one command per line. |
| } | ||
|
|
||
| /** | ||
| * Print all pages in the cache. |
There was a problem hiding this comment.
| * Print all pages in the cache. | |
| * Print the name of all pages in the index. |
| } | ||
|
|
||
| /** | ||
| * Print all pages. |
There was a problem hiding this comment.
| * Print all pages. | |
| * Print the name of all pages. |
There was a problem hiding this comment.
What is the difference between
Promise<any>andPromise<unknown>?
See https://stackoverflow.com/questions/51439843/unknown-vs-any. In summary:
More verbosely, unknown is I don't know (yet), thus I have to figure it out, any is I don't care, thus I don't care
I think there are too many anys/unknowns here, but we can change them later if we need.
agnivade
left a comment
agnivade
left a comment
There was a problem hiding this comment.
I still see my comments not addressed yet. Feel free to request a review once that's done. Thanks.
|
IMO we can take over this PR, since it was opened 2 years ago with no response since then. |
6 participants
Description
Please explain the changes you made here.
Checklist
Please review this checklist before submitting a pull request.
npm run test:all)