Editorial: For `CONTRIBUTING.md`, add explicit references to patterns we've evolved over time #10392

domfarolino · 2024-06-04T16:05:05Z

Recently I spoke with several people who were frustrated with style nits that editors would give them on PRs. I think part of this problem can be ameliorated by simply documenting several of these patterns that we've implicitly evolved over time, that seem to exist in the heads of only a few, so that newcomers have a better shot at writing the correct thing the first time.

domfarolino

Hey @annevk and @domenic I'd love your feedback on whether the things I enumerate here have a place in this document. I think they would be good additions, especially for newcomers, and it gives us much needed clarity on a lot of style decisions that have never been documented so far (I think).

domfarolino · 2024-06-04T16:39:31Z

CONTRIBUTING.md

+ - Use the **"run these steps"** convention to describe what an algorithm that starts with "To", does. [Example #1](https://html.spec.whatwg.org/C#parse-a-url); [Example #2](https://html.spec.whatwg.org/C#create-a-potential-cors-request).
+ - **"If foo, then bar"** instead of "If foo, bar".  [Example](https://github.com/whatwg/html/pull/10269#discussion_r1568114777).
+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.


I'm not sure I feel comfortable linking to random WICG spec discussion as "justification" for anything in this document. But I did so initially just for good archaeological reasons, if we ever wanted to find prior justification for these things voiced by HTML editors wherever it happened to emerge in the past.

But if we want to cut out all of that history and just make the stuff in this document the new gospel, I'm fine with that too. Just let me know what you prefer, but for now I've added as much context as I could find just for good measure.

In general I think inlining the examples instead of, or maybe in addition to, linking to them is best. And I don't think history stuff is necessary.

jyasskin

Thanks for adding this!

jyasskin · 2024-06-04T17:14:16Z

CONTRIBUTING.md

+ - **"If foo, then bar"** instead of "If foo, bar".  [Example](https://github.com/whatwg/html/pull/10269#discussion_r1568114777).
+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.
+ - **Usage of positional, optional, and named[^1] (i.e., linkable) parameters**. See [this logic](https://docs.google.com/document/d/1yxnzjRDVmAR5CC9GcAyY448lBD0u0E98eUEMHDhx1Dw/edit?disco=AAAAeXYly54) for how to order and refer to these.


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.

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

jyasskin · 2024-06-04T17:19:22Z

CONTRIBUTING.md

+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.
+ - **Usage of positional, optional, and named[^1] (i.e., linkable) parameters**. See [this logic](https://docs.google.com/document/d/1yxnzjRDVmAR5CC9GcAyY448lBD0u0E98eUEMHDhx1Dw/edit?disco=AAAAeXYly54) for how to order and refer to these.
+ - **Nesting 3+ conditions** in an "if all of the following are true" clause, for readability. [Example](https://github.com/whatwg/html/pull/9778#discussion_r1540615160).


These bullets should probably be imperative.

If an 'if' has at least 3 conditions, write them as: \``` <li> If all the following are true: <ul> <li>Condition 1</li> <li>Condition 2</li> <li>Condition 3</li> </ul> Then do a thing. </li> \```

I think an example here is useful, in particular because the punctuation is not quite like @jyasskin suggests. See be6c665

jyasskin · 2024-06-04T17:21:50Z

CONTRIBUTING.md

+ - Use the **"run these steps"** convention to describe what an algorithm that starts with "To", does. [Example #1](https://html.spec.whatwg.org/C#parse-a-url); [Example #2](https://html.spec.whatwg.org/C#create-a-potential-cors-request).
+ - **"If foo, then bar"** instead of "If foo, bar".  [Example](https://github.com/whatwg/html/pull/10269#discussion_r1568114777).
+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.


I'm uncomfortable with this particular convention living in HTML's CONTRIBUTING.md without being defined in Infra. However, it's better to put it here than have it just in some editors' heads. So, carry on with your incremental improvement.

domenic

Thanks very much for doing this!

CONTRIBUTING.md

domenic · 2024-06-05T01:51:07Z

CONTRIBUTING.md

+
+### Misc wording
+
+ - Use the **"run these steps"** convention to describe what an algorithm that starts with "To", does. [Example #1](https://html.spec.whatwg.org/C#parse-a-url); [Example #2](https://html.spec.whatwg.org/C#create-a-potential-cors-request).


Using "run these steps" is only desirable if you state the return type.

I think https://infra.spec.whatwg.org/#algorithm-declaration covers all of this pretty well. Maybe just link to it while noting that

Due to the long legacy of the existing text the guidelines below are not always applied.

applies here too?

Using "run these steps" is only desirable if you state the return type.

Oh interesting, I didn't know about this distinction. I guess it is covered in infra (here vs here).

I guess this is because when you have an algorithm with a return type, the return type is declared in the sentence after the algorithm declaration, so having the algorithm declaration end with : which immediately proceeds a non-algorithm sentence (about the return type) would be weird.

Anyways, I'm torn on what to do here. On one hand I'm not sure linking to that specific part of the infra spec is useful here (why that one part specifically instead of other parts?). On the other hand, I feel like this is one of the more common style nits I see, so maybe linking to it specifically is a good idea.

Maybe the right move here is to, inside CONTRIBUTING.MD, make a # Style guide section and do the following:

Just link to both Infra & https://whatwg.org/style-guide (which I vaguely knew existed but could not find a link to anywhere). This are useful pointers for new contributors.

Below that, list a few specific style callouts like this one, that we find ourselves addressing more frequently than others.

I like calling out frequent mistakes and referring to Infra. I'm not sure the current phrasing is accurate though. Maybe just something like "Use the algorithm declaration conventions explained in Infra".

domenic · 2024-06-05T01:52:22Z

CONTRIBUTING.md

+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.
+ - **Usage of positional, optional, and named[^1] (i.e., linkable) parameters**. See [this logic](https://docs.google.com/document/d/1yxnzjRDVmAR5CC9GcAyY448lBD0u0E98eUEMHDhx1Dw/edit?disco=AAAAeXYly54) for how to order and refer to these.
+ - **Nesting 3+ conditions** in an "if all of the following are true" clause, for readability. [Example](https://github.com/whatwg/html/pull/9778#discussion_r1540615160).


I think an example here is useful, in particular because the punctuation is not quite like @jyasskin suggests. See be6c665

domenic · 2024-06-05T01:52:50Z

CONTRIBUTING.md

+ - Use the **"run these steps"** convention to describe what an algorithm that starts with "To", does. [Example #1](https://html.spec.whatwg.org/C#parse-a-url); [Example #2](https://html.spec.whatwg.org/C#create-a-potential-cors-request).
+ - **"If foo, then bar"** instead of "If foo, bar".  [Example](https://github.com/whatwg/html/pull/10269#discussion_r1568114777).
+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.


In general I think inlining the examples instead of, or maybe in addition to, linking to them is best. And I don't think history stuff is necessary.

CONTRIBUTING.md

domenic · 2024-06-05T01:55:08Z

CONTRIBUTING.md

@@ -93,3 +103,22 @@ is not indented, but
 is.

 End tags must not be omitted (except where it is consistent to do so) and attribute values must be quoted (use double quotes).
+
+### Misc wording


I think most of this should live in either https://whatwg.org/style-guide or Infra, but we could start with putting things here when they don't already exist in those two places.

(Source formatting stuff clearly belongs here though.)

Yeah that seems reasonable to me, but also see #10392 (comment) where I discuss the possibility of mentioning things that are redundant with other style guides, but frequent sources of style nits in review.

domenic · 2024-06-05T01:57:24Z

CONTRIBUTING.md

+ - **"If foo, then bar"** instead of "If foo, bar".  [Example](https://github.com/whatwg/html/pull/10269#discussion_r1568114777).
+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.
+ - **Usage of positional, optional, and named[^1] (i.e., linkable) parameters**. See [this logic](https://docs.google.com/document/d/1yxnzjRDVmAR5CC9GcAyY448lBD0u0E98eUEMHDhx1Dw/edit?disco=AAAAeXYly54) for how to order and refer to these.


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

CONTRIBUTING.md

domenic · 2024-06-24T07:22:33Z

CONTRIBUTING.md

+
+### Misc wording
+
+ - Use the **"run these steps"** convention to describe what an algorithm that starts with "To", does. [Example #1](https://html.spec.whatwg.org/C#parse-a-url); [Example #2](https://html.spec.whatwg.org/C#create-a-potential-cors-request).


I like calling out frequent mistakes and referring to Infra. I'm not sure the current phrasing is accurate though. Maybe just something like "Use the algorithm declaration conventions explained in Infra".

domenic · 2024-06-24T07:23:13Z

CONTRIBUTING.md

+ - Use the **"run these steps"** convention to describe what an algorithm that starts with "To", does. [Example #1](https://html.spec.whatwg.org/C#parse-a-url); [Example #2](https://html.spec.whatwg.org/C#create-a-potential-cors-request).
+ - **"If foo, then bar"** instead of "If foo, bar".  [Example](https://github.com/whatwg/html/pull/10269#discussion_r1568114777).
+ - **"Abort these steps" vs "return"**.
+   - We've passively evolved the pattern of reserving "return" for exiting algorithms and methods, and "abort these steps" for terminating a set of substeps / [in parallel](https://html.spec.whatwg.org/C#in-parallel) steps without messing with the control flow of the "outer" procedure. See [this logic](https://github.com/WICG/portals/pull/138#discussion_r292649287), as well as https://github.com/whatwg/infra/issues/258 and https://github.com/whatwg/infra/pull/255.


Nit: this doesn't seem like a sub-bullet; it can just be part of the bullet itself.

Add explicit references to patterns we've passively adapted

432a072

domfarolino changed the title ~~Editorial: For CONTRIBUTING.md, add explicit references to patterns we've passively evolved over time~~ Editorial: For CONTRIBUTING.md, add explicit references to patterns we've evolved over time Jun 4, 2024

domfarolino added 2 commits June 4, 2024 18:06

Go back

b660978

Fix some stuff

46692de

domfarolino marked this pull request as ready for review June 4, 2024 16:37

domfarolino requested review from domenic and annevk June 4, 2024 16:37

domfarolino commented Jun 4, 2024

View reviewed changes

jyasskin reviewed Jun 4, 2024

View reviewed changes

domenic reviewed Jun 5, 2024

View reviewed changes

Re-structure

c36fd48

domfarolino mentioned this pull request Jun 5, 2024

Specifically list e.g., and i.e., in the grammar section whatwg/whatwg.org#447

Merged

domenic reviewed Jun 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: For `CONTRIBUTING.md`, add explicit references to patterns we've evolved over time #10392

Editorial: For `CONTRIBUTING.md`, add explicit references to patterns we've evolved over time #10392

domfarolino commented Jun 4, 2024

domfarolino left a comment

domfarolino Jun 4, 2024

domenic Jun 5, 2024

jyasskin left a comment

jyasskin Jun 4, 2024

domenic Jun 5, 2024

jyasskin Jun 4, 2024

domenic Jun 5, 2024

jyasskin Jun 4, 2024

domenic left a comment

domenic Jun 5, 2024

domfarolino Jun 5, 2024

domenic Jun 24, 2024

domenic Jun 5, 2024

domenic Jun 5, 2024

domenic Jun 5, 2024

domfarolino Jun 5, 2024

domenic Jun 5, 2024

domenic Jun 24, 2024

domenic Jun 24, 2024


		### Misc wording

		- Use the "run these steps" convention to describe what an algorithm that starts with "To", does. [Example #1](https://html.spec.whatwg.org/C#parse-a-url); [Example #2](https://html.spec.whatwg.org/C#create-a-potential-cors-request).

Editorial: For CONTRIBUTING.md, add explicit references to patterns we've evolved over time #10392

Are you sure you want to change the base?

Editorial: For CONTRIBUTING.md, add explicit references to patterns we've evolved over time #10392

Conversation

domfarolino commented Jun 4, 2024

domfarolino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyasskin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Editorial: For `CONTRIBUTING.md`, add explicit references to patterns we've evolved over time #10392

Editorial: For `CONTRIBUTING.md`, add explicit references to patterns we've evolved over time #10392