Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Editorial: For
CONTRIBUTING.md
, add explicit references to patterns we've evolved over time #10392Editorial: For
CONTRIBUTING.md
, add explicit references to patterns we've evolved over time #10392Changes from 3 commits
432a072
b660978
46692de
c36fd48
f8c29ef
59e3994
09708c4
15e8fe3
8ff83e2
a81535b
a5fa681
de96448
07c6161
5a26846
98c787e
1cf2afe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The
?disco=...
link only goes to a comment for people with permission to add comments. (Yay Docs permissions.) I'd argue for inlining all the references-to-discussions into this document.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.
+1 on inlining. And figuring out how to state it more generally.
Probably https://infra.spec.whatwg.org/#algorithm-params should be referenced from here. But that doesn't give too much guidance on when to use mandatory vs. named-mandatory vs. optional vs. named-optional.
I'm not sure how strong the guidance should be here, but the doc comment you link to is indeed a reasonable default for people to start from.
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.
Looking at this a little bit more, I'm not even sure that the current version of
#navigate
is consistent with the guidance in the Google doc (there seem to be no optional non-named/linkable parameters in that algorithm declaration, which honestly just seems for the best).I've simplified this by linking to the infra link @domenic provided above and specifically calling out the fact that you must use named/linkable optional parameters whenever a callsite wants to pass in a later-positioned optional argument without earlier-positioned ones. I think that's reasonable advice, although I'm not sure it's really a. The alternative would be to just say use named params for all optional params, which I think I would prefer (we have too many algorithms that are impossible to audit the callsite/declaration conformance of, in part due to lack of linkability), but that's a bit heavy-handed so I'm not sure how others feel. WDYT about the current version?