-
Notifications
You must be signed in to change notification settings - Fork 100
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
Specify optional and named parameters #325
Conversation
9124f24
to
83e5008
Compare
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.
This looks great, thanks for formalizing this!
I'm not entirely convinced we want the last paragraph however. Making that tradeoff seems extremely nuanced and I'd rather folks either use a struct or not name the parameters.
"<code>form submission</code>". | ||
|
||
<li><a href=#example-navigate-algo-positional>Navigate</a> to <var ignore>resource</var> with | ||
"<code>form submission</code>" and true. |
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.
Feels like we should call out positional booleans as bad practice.
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.
If we really want to do that I guess that would further argue for special casing booleans in some ways.
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.
Yeah, I dunno, it's OK-ish especially since it's all hypertext, but I agree generally it's not going to be a good time, and you probably want your booleans to be named.
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.
It seems based on the discussion we should change this too, right? To only have examples with recommended practice.
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.
Maybe. I'm unsure because it's part of the "narrative flow" of the section: it shows the algorithm evolving from unclear and inflexible optional positional arguments, to clear and flexible optional named arguments.
We could instead specifically explain that not only is this non-ideal because of the ordering restrictions, but also because of the presence of non-named boolean parameters?
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.
Yeah that sounds good to me. I suspect that would work for @jyasskin too.
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.
Done.
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.
Yep, thanks. Note that this example-of-things-we-don't-want-people-to-do winds up looking the same as all the other examples-of-things-to-do. Vanilla bikeshed supports a heading="Bad Example"
attribute to make it a little clearer, but that attribute doesn't work with the WHATWG style sheet's .example::before { content: 'Example'; }
rule.
infra.bs
Outdated
<p>Non-optional named parameters may also be used, using the same convention of marking them up as | ||
both variables and definitions, and linking to them from call sites. However, one should default to | ||
making non-optional parameters positional, for simplicity and uniformity. Non-optional named | ||
parameters should only be used when they significantly improve clarity, and even then it might be |
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.
FWIW, I think Call foo with 3
and Call foo with "navigate"
are not much better than Call foo with false
, and Infra should encourage people to name those non-optional parameters. Perhaps:
<p>Non-optional named parameters may also be used, using the same convention of marking them up as | |
both variables and definitions, and linking to them from call sites. However, one should default to | |
making non-optional parameters positional, for simplicity and uniformity. Non-optional named | |
parameters should only be used when they significantly improve clarity, and even then it might be | |
<p>Non-optional parameters may also be named, using the same convention of marking them up as | |
both variables and definitions, and linking to them from call sites. Do this when it improves | |
clarity at call sites. It is also |
"<code>form submission</code>". | ||
|
||
<li><a href=#example-navigate-algo-positional>Navigate</a> to <var ignore>resource</var> with | ||
"<code>form submission</code>" and true. |
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.
Yep, thanks. Note that this example-of-things-we-don't-want-people-to-do winds up looking the same as all the other examples-of-things-to-do. Vanilla bikeshed supports a heading="Bad Example"
attribute to make it a little clearer, but that attribute doesn't work with the WHATWG style sheet's .example::before { content: 'Example'; }
rule.
So, this has been sitting for a while. I'd like to get it landed. Outstanding feedback:
@annevk, any thoughts? And if anyone wants to chime with their perspective on the above, feel free to do so. |
I think encouraging naming for optional arguments is reasonable, but I'm okay with not endorsing anything for now, especially as there is no tooling support either. We could use |
With |
979c69d
to
5cf09a1
Compare
Alright, this might be ready to go then! |
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!
Co-authored-by: Jeffrey Yasskin <jyasskin@google.com>
Closes #320.
Preview | Diff