-
Notifications
You must be signed in to change notification settings - Fork 636
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
docs(fmt): improve API docs #4829
Conversation
@@ -916,11 +916,23 @@ class Printf { | |||
} | |||
|
|||
/** | |||
* Converts and format a variable number of `args` as is specified by `format`. | |||
* Converts and formats a variable number of `args` as is specified by `format`. |
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.
Mention that users should look at the printf
module level doc to view what format strings are supported.
@crowlKats we should really have some way to link to module level docs with @link
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.
@lucacasonato I can look into it on friday
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 want to wait until we can link to the module level doc before this PR lands? I think other than that the PR looks good to me
Co-authored-by: Luca Casonato <lucacasonato@yahoo.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4829 +/- ##
=======================================
Coverage 92.35% 92.35%
=======================================
Files 488 488
Lines 41580 41581 +1
Branches 5405 5405
=======================================
+ Hits 38402 38403 +1
Misses 3122 3122
Partials 56 56 ☔ View full report in Codecov by Sentry. |
fmt/printf.ts
Outdated
* | ||
* @param format The format string to use | ||
* @param args The arguments to format | ||
* @returns formatted string |
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.
nit: in other places of @returns
tag the sentences start with The
, but here it doesn't
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.
Some nits. Otherwise, LGTM! Good to see this done.
* @example Usage | ||
* ```ts | ||
* import { reset } from "@std/fmt/colors"; | ||
* console.log(reset("Hello, world!")); |
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.
Non-blocking nit: I think we should have at least one empty line between imports and the rest of the code, for readability. Ditto elsewhere.
fmt/colors.ts
Outdated
* @param str text to reset | ||
* @returns text with reset color |
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.
Nit: We should use proper casing. Ditto elsewhere.
* @param str text to reset | |
* @returns text with reset color | |
* @param str Text to reset | |
* @returns Text with reset color |
@@ -117,23 +143,47 @@ function run(str: string, code: Code): string { | |||
|
|||
/** | |||
* Reset the text modified. |
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.
We should use "printed text" in the JSDocs for these descriptions and in tags. Such emphasis might be valuable to beginners.
part of #3764