-
Notifications
You must be signed in to change notification settings - Fork 3k
Source reformatting: switch to no line-breaking #11640
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
base: main
Are you sure you want to change the base?
Conversation
The reformatting tool supports opting out for specific elements with a |
Very cool! The other major section to not reformat is the acknowledgements section. Is there any way to do some sort of programmatic check that only whitespace has changed, to rule out any content-deleting bugs in the reformatter? Maybe just the htmldiff tool would work? |
In the paragraph that begins "Here is an example of color space conversion", the status quo: (This is the only case where the status quo has a line-break after an attribute's |
Arguably that's a bug in the status quo since you're supposed to only wrap on where whitespace would "naturally" occur. |
c7dbbc0
to
d81a7bb
Compare
@jmdyck fixed the
Thanks!
I've marked elements I found with custom indentation with
Shows "Only whitespaces differences" after running the script. |
c8b3408
to
bc61fba
Compare
I think I'm happy with the output now. LMK if there are more elements that should have |
Would you prefer a separate PR to fix such bugs? |
As far as I can tell we only have one instance. We could fix it separately or incorporate it in a random PR. I don't think it matters much? |
No need to fix |
There's only one instance of 'newline after equals', yes, but there's more than one instance of 'newline where whitespace wouldn't "naturally" occur'. |
In this PR, there are many cases where, if the first non-blank on a line is an element, it isn't joined up with the previous line, but presumably should be. (If you're looking for examples, searching for |
@jmdyck, thanks, fixed now. |
f91f5c0
to
4dece04
Compare
@zcorpan it seems this branch still has conflicts? |
4dece04
to
6659c3f
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.
Rubber stamp, assuming this is going to be followed up with a "Meta:" commit to fix blame and hoping that this doesn't introduce new issues.
|
Thanks, these should be fixed now.
Yes, comments that are preceded with a newline and followed by a newline are considered structural and left alone (including internal linebreaks). Other comments are reflowed inline. This means some comments still have internal linebreaks that could arguably have internal linebreaks removed, but I think we can leave them as-is. |
ddabc39
to
e3e813a
Compare
I'm happy with the result now. I've looked through the source and added some more |
e3e813a
to
2ac969d
Compare
I noticed multiple spaces before comments. Edit: fixed. |
2ac969d
to
ebd8e22
Compare
<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 | ||
``` | ||
You can run https://github.com/zcorpan/reformahtml locally to remove inter-paragraph line wrapping. This script is also run as a GitHub workflow. |
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.
You can run https://github.com/zcorpan/reformahtml locally to remove inter-paragraph line wrapping. This script is also run as a GitHub workflow. | |
You can run https://github.com/zcorpan/reformahtml locally to remove intra-paragraph line wrapping. This script is also run as a GitHub workflow. |
I'm puzzled about the semantics of E.g., in (Mind you, I think that's the only example where |
There are some spots where the reformatter has incorrectly dropped a space before a start-tag, e.g.
|
There are some spots where the reformatter leaves an end-tag on a line by itself, where it should probably append it to the previous line, e.g.:
|
Thanks. In the source, there are 4 cases where two comments are immediately adjacent. (Scan for |
Sorry to jump in late, but I'm still not sure what the value add here is. Is it just lower friction for contributors to not have to worry about line breaks? What is the rule we're trying to enforce after this PR? Is it:
I think I still prefer the 100 character line break rule, and using a tool like |
The rules are not consistent though. Most other WHATWG specifications don't permit breaking on every possible space. Moving towards wrapping in your editor seems acceptable to me and if this works out we can port it to DOM, Fetch, etc. |
Generally, no line breaks within a paragraph are allowed. Exceptions are "custom" linebreaks for readability and easier editing, e.g. HTML uses one line per name in the Acknowledgements paragraph. These must have a The benefit of this approach is that contributors don't have to use a tool like specfmt, but instead just write the paragraph and then commit. The current line-wrapping rule for HTML also makes searching for terms annoying, which is fixed by this change. |
ebd8e22
to
81cc2a5
Compare
@jmdyck thank you for reviewing!
This was a bug in the script, now fixed.
These were due to inconsistent formatting in
Fixed. |
I'm still puzzled about the semantics of At line 65870 of
In the reformatted result (line 48421), these lines don't get joined up, but they probably should be. Generally, the reformatter squashes runs of spaces down to a single space, but there are a few places where it doesn't:
|
* Add data-noreformat attributes * Fix double spaces and incorrect wrapping around tags * Add a Check Formatting workflow * Update CONTRIBUTING.md
This applies reformahtml 0.1.9. See https://github.com/zcorpan/reformahtml Fixes #1059.
81cc2a5
to
ef51b44
Compare
@jmdyck thanks, I think those are also fixed now. |
Okay, I think this is ready to go. |
What are some good, recommended editor settings to visually wrap existing long lines to ~100 characters, and preserve the indentation level? Specifically for vim, I can't find a good way to visually wrap long lines tighter than the vim window itself, which leaves the spec pretty unreadable to me. Does anyone know the trick in vim? |
:set wrap linebreak breakindent breakindentopt=shift:1 will visually wrap long lines at word-boundaries and indent the wrapped lines at 1 more than the indent of the actual line. But the wrapping-limit is still defined by the width of the window -- I don't think you can visually wrap to a specified width, but I might have missed something. |
Yeah, this is the problem I’m having. |
For people who use VS Code, I found that you can set
I think we'd have to live with this for now, but can file a request to GitHub to improve the wrapping. It should be possible in CSS (but a bit ugly without full
I think this is true. A workaround could be to quote the part of the line you want to comment on. We can also file another request for GitHub to support review comments on part of a line. I also note that some specs e.g. https://github.com/whatwg/compression and https://github.com/whatwg/urlpattern already use no line breaks and it seems to me this hasn't been an issue for PR reviews. |
Meta: Preparation for source reformatting
Meta: Source reformatting: switch to no line-breaking
This applies reformahtml 0.1.6. See https://github.com/zcorpan/reformahtml
Fixes #1059.
/acknowledgements.html ( diff )
/browsers.html ( diff )
/browsing-the-web.html ( diff )
/canvas.html ( diff )
/common-dom-interfaces.html ( diff )
/common-microsyntaxes.html ( diff )
/comms.html ( diff )
/custom-elements.html ( diff )
/dnd.html ( diff )
/document-lifecycle.html ( diff )
/document-sequences.html ( diff )
/dom.html ( diff )
/dynamic-markup-insertion.html ( diff )
/edits.html ( diff )
/embedded-content-other.html ( diff )
/embedded-content.html ( diff )
/form-control-infrastructure.html ( diff )
/form-elements.html ( diff )
/forms.html ( diff )
/grouping-content.html ( diff )
/iana.html ( diff )
/iframe-embed-object.html ( diff )
/image-maps.html ( diff )
/imagebitmap-and-animations.html ( diff )
/images.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/introduction.html ( diff )
/links.html ( diff )
/media.html ( diff )
/microdata.html ( diff )
/named-characters.html ( diff )
/nav-history-apis.html ( diff )
/obsolete.html ( diff )
/parsing.html ( diff )
/popover.html ( diff )
/references.html ( diff )
/rendering.html ( diff )
/scripting.html ( diff )
/sections.html ( diff )
/semantics-other.html ( diff )
/semantics.html ( diff )
/server-sent-events.html ( diff )
/speculative-loading.html ( diff )
/structured-data.html ( diff )
/syntax.html ( diff )
/system-state.html ( diff )
/tables.html ( diff )
/text-level-semantics.html ( diff )
/timers-and-user-prompts.html ( diff )
/urls-and-fetching.html ( diff )
/web-messaging.html ( diff )
/webappapis.html ( diff )
/webstorage.html ( diff )
/workers.html ( diff )
/worklets.html ( diff )
/xhtml.html ( diff )