Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
432a072
Add explicit references to patterns we've passively adapted
domfarolino Jun 4, 2024
b660978
Go back
domfarolino Jun 4, 2024
46692de
Fix some stuff
domfarolino Jun 4, 2024
c36fd48
Re-structure
domfarolino Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,21 @@ On the other hand, we want to especially keep:

In between these clear-cut categories, there is some gray area. Please feel free to open an issue if you think something is being included that shouldn't be, or is being excluded but should be kept.

## Source formatting
## Style guide

Due to the long legacy of the existing text the guidelines below are not always applied. We do require that you apply the guidelines when making changes, though are happy to provide assistance if this proves to be a blocker to you.
The HTML Standard generally follows style conventions listed in the [Infra Standard](https://infra.spec.whatwg.org) and the [WHATWG style guide](https://whatwg.org/style-guide). Additionally, the HTML Standard follows some specific style conventions not captured by those documents, that we enumerate below.

Due to the long legacy of the existing text, these guidelines are not always applied. We do require that you apply the guidelines when making changes, though are happy to provide assistance if this proves to be a blocker to you.

### Source formatting


#### Line wrapping length

Use a column width of 100 characters and add newlines where whitespace is used. (Emacs, set `fill-column` to `100`; in Vim, set `textwidth` to `100`; and in Sublime, set `wrap_width` to `100`. Alternatively, wrap the paragraph(s) with your changes with https://domenic.github.io/rewrapper/. Make sure that `column length to rewrap` is set to 100.)

#### Wrapping opportunities

Using newlines between "inline" element tag names and their content is forbidden. (This actually alters the content, by adding spaces.) That is,
```html
<dd><span>Parse error</span>. Create a new DOCTYPE token. Set its <i data-x="force-quirks
Expand All @@ -60,8 +69,14 @@ is fine and
```
is not.

Using newlines between attributes and inside attribute values that contain whitespace is allowed.
Always wrap after putting the maximum number of characters on a single line within these guidelines.
Using newlines between attributes and inside attribute values that contain whitespace is allowed. Always wrap after putting the maximum number of characters on a single line within these guidelines.

```html
<p>A <code>base</code> element that is the first <code>base</code> element with an <code
data-x="attr-base-href">href</code> content attribute <span>in a document tree</span> has a
```

### Element hierarchy

An `<li>` element always has a `<p>` element inside it, unless it's a child of `<ul class="brief">`.

Expand Down Expand Up @@ -93,3 +108,18 @@ 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).

### Common mistakes around prose style

This section lists style conventions that are typically covered by Infra or the WHATWG style guide, but that are nevertheless frequent sources of style nits by editors of the HTML Standard.

- 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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. Below that, list a few specific style callouts like this one, that we find ourselves addressing more frequently than others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- **"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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- **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.
Copy link
Member

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.

Copy link
Member

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.

- **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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These bullets should probably be imperative.

If an 'if' has at least 3 conditions, write them as:

\```
<li>
 <p>If all the following are true:</p>
 <ul>
  <li><p>Condition 1</p></li>
  <li><p>Condition 2</p></li>
  <li><p>Condition 3</p></li>
 </ul>
 <p>Then do a thing.</p>
</li>
\```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- **Conjugating algorithm invocations inline** so they read more naturally in English, instead of more procedurally. [Example](https://github.com/whatwg/html/pull/9778#discussion_r1574075112).
- Prefer American English to British English; see the [WHATWG style guide](https://whatwg.org/style-guide).

[^1]: For example, see parameters like https://html.spec.whatwg.org/C#navigation-referrer-policy, which are named/linkable parameters in an algorithm's declaration.
Loading