add jsdocs to tldr and cache #355

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

Jump to bottom

Open

navarroaxel wants to merge 1 commit into main

from docs/add-jsdocs

Open

add jsdocs to tldr and cache #355

navarroaxel wants to merge 1 commit into main from docs/add-jsdocs

Conversation

@navarroaxel

Copy link

Contributor

@navarroaxel navarroaxel commented May 25, 2021 •

edited

Loading

Description

Please explain the changes you made here.

Checklist

Please review this checklist before submitting a pull request.

Code compiles correctly
Created tests, if possible
All tests passing (npm run test:all)
Extended the README / documentation, if necessary

@navarroaxel


 add jsdocs to tldr and cache

7776bb4

blueskyson

blueskyson approved these changes

Jul 16, 2023

View reviewed changes

Copy link

Member

@blueskyson blueskyson left a comment

Choose a reason for hiding this comment

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

Wow, I wonder why comment enhancements are left so long unmerged.

@blueskyson blueskyson requested review from MasterOdin and agnivade

July 16, 2023 06:16

kbdharun

kbdharun approved these changes

Jul 16, 2023

View reviewed changes

agnivade

agnivade reviewed

Jul 17, 2023

View reviewed changes

lib/cache.js

/**

* Fetch a page from cache using preferred language and preferred platform.

* @param page {} A

* @returns {Promise<unknown>}

Copy link

Member

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

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

What is the difference between Promise<any> and Promise<unknown> ?

lib/index.js

Comment on lines 118 to 119

// Return all commands for a given platform.

// P.S. - The platform 'common' is always included.

Copy link

Member

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

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

Should we remove this then? Seems repeated.

lib/tldr.js

}

/**

* Print pages for a given platform..

Copy link

Member

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

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

Multiple periods at the end of the sentence.

lib/tldr.js

/**

* Print a command page.

* @param commands {string[]} A given command to be printed.

Copy link

Member

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

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

"A given command" is incorrect when we are passing an array. Let's change this to plural.

lib/tldr.js

/**

* Update the cache.

* @returns {Promise<any>} A promise with the index.

Copy link

Member

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

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

Should this be "A promise with the cache"?

@kbdharun kbdharun changed the base branch from master to main

October 3, 2023 17:12

vitorhcl

vitorhcl approved these changes

Mar 17, 2024

View reviewed changes

Copy link

Member

@vitorhcl vitorhcl left a comment

Choose a reason for hiding this comment

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

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.

lib/tldr.js

}

/**

* Print pages for a given platform..

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* Print pages for a given platform..

* Print pages for a given platform.

lib/tldr.js

/**

* Print a command page.

* @param commands {string[]} A given command to be printed.

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* @param commands {string[]} Agivencommand to be printed.

* @param commands {string[]} Givencommands to be printed.

lib/index.js

* Return all commands for a given platform.

* The 'common' platform is always included.

* @param platform {string} The platform.

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* @param platform {string} The platform.

* @param platform {string} The desiredplatform.

lib/tldr.js

}

/**

* Print a random page.

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* Print a random page.

* Print a random pageexample.

lib/cache.js

}

/**

* Fetch stats from the cache folder.

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* Fetch stats from the cache folder.

* Fetch stats from the cache folderforgettingitslastmodifiedtime(mtime).

lib/index.js

// 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.

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* @returns {boolean} Abooleanthatindicatesifthe page ispresent.

* @returns {boolean} Thepresenceofthe page intheindex.

lib/cache.js

}

/**

* Update the cache folder and returns the English entries.

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* Update the cache folder andreturnsthe Englishentries.

* Update the cache folder usingatemporarydirectory,updatethe indexandreturnit.

lib/tldr.js

/**

* Print pages for a given platform..

* @param singleColumn {boolean} A boolean to print one command per line.

Copy link

Member

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

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

Suggested change

* @param singleColumn {boolean} Abooleantoprint one command per line.

* @param singleColumn {boolean} Print one command per line.

lib/tldr.js

}

/**

* Print all pages in the cache.

Copy link

Member

@vitorhcl vitorhcl Mar 17, 2024

Choose a reason for hiding this comment

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

Suggested change

* Print all pages in the cache.

* Print thenameofall pages in the index.

lib/tldr.js

}

/**

* Print all pages.

Copy link

Member

@vitorhcl vitorhcl Mar 17, 2024

Choose a reason for hiding this comment

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

Suggested change

* Print all pages.

* Print thenameofall pages.

vitorhcl

vitorhcl reviewed

Mar 17, 2024

View reviewed changes

Copy link

Member

@vitorhcl vitorhcl left a comment •

edited

Loading

Choose a reason for hiding this comment

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

#355 (comment):

What is the difference between Promise<any> and Promise<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.

@vitorhcl vitorhcl requested review from blueskyson and kbdharun

March 19, 2024 18:20

@kbdharun kbdharun requested a review from agnivade

March 20, 2024 01:45

agnivade

agnivade reviewed

Mar 20, 2024

View reviewed changes

Copy link

Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

I still see my comments not addressed yet. Feel free to request a review once that's done. Thanks.

@vitorhcl

Copy link

Member

vitorhcl commented Mar 20, 2024

IMO we can take over this PR, since it was opened 2 years ago with no response since then.

Labels

None yet

5 participants

@navarroaxel @vitorhcl @agnivade @kbdharun @blueskyson

Uh oh!

Uh oh!

add jsdocs to tldr and cache #355

Are you sure you want to change the base?

add jsdocs to tldr and cache #355

Uh oh!

Conversation

@navarroaxel navarroaxel commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

@blueskyson blueskyson left a comment

Choose a reason for hiding this comment

Uh oh!

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

@agnivade agnivade Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl left a comment

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 16, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl Mar 17, 2024

Choose a reason for hiding this comment

Uh oh!

@vitorhcl vitorhcl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@agnivade agnivade left a comment

Choose a reason for hiding this comment

Uh oh!

vitorhcl commented Mar 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

@navarroaxel navarroaxel commented May 25, 2021 •

edited

Loading

@vitorhcl vitorhcl left a comment •

edited

Loading