-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
docs: document asset utilities #13355
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
Conversation
🦋 Changeset detectedLatest commit: 49f4c41 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
That's great. However, I have a few suggestions.
Co-authored-by: Junseong Park <[email protected]>
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 for documenting these Ema! Just a few quick comments from me!
| * Checks if the given URL's port matches the specified port. If no port is provided, it returns true. | ||
| * | ||
| * @param {URL} url - The URL object whose port will be checked. | ||
| * @param {string} [port] - The port to match against the URL's port. Optional. |
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.
Just wondering whether you already have a precedent for how you show optional parameters and suggesting that you be consistent with that that if it exists.
In docs, for example, I think we tend to "front-load" the optional part, or even if we don't, it's usually lowercase and in parenthesis:
* @param {string} [port] - (optional) The port to match against the URL's port.
Not suggesting you need to change this! Only a reminder that if you do have other optional params already written, it's nice if they're written the same way consistently!
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.
As per JSDoc, a parameter is optional when is surrounded in square brackets: https://jsdoc.app/tags-param#optional-parameters-and-default-values
In this example, the variable port is optional because it's written [port]. Unfortunately, the IDEs aren't very good at this, and they don't render it as optional
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.
OK, not opposed to adding Optional. at the end of the text if you think that's a helpful way to display it! Just wanted to see if you already had a convention to follow because it seems like there would be other optional values, too, and it's always great if things are consistent. I leave it totally up to you!
Co-authored-by: Sarah Rainsberger <[email protected]>
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.
LGTM! We might might discover that when it comes to putting these in docs, we adjust slightly depending on how things look. But all this looks great here!
Changes
This PR documents the asset utilities that we expose to users.
It seems that these aren't properly documented, so I wanted to start from here, and then send a PR to docs to make it official.
Testing
N/A
Docs
FYI, I helped myself with an AI tool, so I would appreciate a second pair of eyes. I already checked what the AI tool wrote before sending the PR.
/cc @withastro/maintainers-docs for feedback!