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.

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

Avoid the 100 char wrap #31

Open
jakearchibald opened this issue Aug 2, 2017 · 29 comments
Open

Avoid the 100 char wrap #31

jakearchibald opened this issue Aug 2, 2017 · 29 comments

Comments

@jakearchibald
Copy link

jakearchibald commented Aug 2, 2017

Negatives to the 100 char wrap:

  • You have to remember to do it.
  • You need to do it manually or get a tool for your editor.
  • A one-word change can turn into a diff of many lines.
  • Makes it difficult to search for a term if the term is newline split in places.

I guess it's a benefit if your text editor / diff viewer doesn't support wrapping, but is that still a valid reason?

We don't manually wrap lines on the service worker spec, and the diffs look pretty good in split mode w3c/ServiceWorker@65cf285?diff=split.

@jakearchibald
Copy link
Author

@tabatkins uses semantic line breaks https://github.com/w3c/csswg-drafts/blob/master/css-align/Overview.bs which I kinda like.

@annevk
Copy link
Member

annevk commented Aug 2, 2017

Do we need semantic if not wrapping at all works just fine?

This has been a frequent issue for new contributors so I support revisiting our approach.

And thanks to @tobie we also have HTML readable diff documents these days, which should also help a lot (although not for whatwg/html at this point).

cc @foolip @domenic @zcorpan

@domenic
Copy link
Member

domenic commented Aug 2, 2017

In the end this can be an individual-editor(s) thing, with just suggested guidelines. So the below is tinged by my preferences, and will impact repositories I am the editor on, but doesn't necessarily apply universally.

I am strongly against semantic wrapping. I'd rather we not derail the thread into a discussion of its merits.

I like machine-enforceable wrapping (either via one's editor or a tool like The Great Re-Wrapper). So I like breaking in the middle of tags, like HTML but unlike DOM/most of @annevk's repositories. I agree searching is a problem with this approach, however.

I don't mind per-paragraph diffs instead of per-line diffs when you change a word. The diffs are still readable (especially on GitHub). It mainly impacts blame, but in my experience blame is already useless for specs unless you do recursive blame, and recursive blame bypasses such changes easily.

I think having a readable default when you open default-configured editors and viewers is important, so I prefer having a limit. Most of my tools do not word-wrap by default, but have to be opted in on a per-file basis. If you're lucky they'll remember it per file.

100 seems nice as it ensures no wrapping on GitHub and isn't too short like 80 or similar.

So in conclusion, I am happy with the status quo, with the only point of contention being whether we wrap in a machine-enforceable fashion vs. leaving terms together.

@jakearchibald
Copy link
Author

@domenic what editor do you use? Many editors support a project level config to enable wrapping for particular file types.

@domenic
Copy link
Member

domenic commented Aug 2, 2017

Sublime Text. The issue is partially about transitioning between different computers and such and remembering to set it on each new one I work on. As well as new contributors.

@foolip
Copy link
Member

foolip commented Aug 2, 2017

I mostly use Emacs which can re-wrap OKish, but it'll also wrap lines together with previous/following lines, so I end up spending time avoiding that by adding/removing blank lines. It's a bit of a nuisance.

However, to get to anything other than the 100 limit we have, I think we'd really need a tool to automatically re-wrap to the new best thing. But if we have that, I'd prefer if we just use that tool to automatically re-wrap to 100, so that one can edit without concern for wrapping and then just run the script. In https://github.com/w3c/webvtt/blob/gh-pages/format.py I hacked something together for WebVTT, but I never worked out some of the edge cases and certainly wouldn't trust it without a lot of testing first.

@jakearchibald
Copy link
Author

@foolip doesn't that leave us with the diff viewing problem, and also the search problem?

@foolip
Copy link
Member

foolip commented Aug 2, 2017

It does. For diffs, if it's a real mess, I always fall back to git diff --word-diff -w and for search, Emacs (and browsers) already searches across lines. That doesn't help for git grep or git log -S though, an occasional headache.

With very long lines, the editor/terminal/diff viewer needs to be more clever to make it readable, and I guess I enjoy that even less than the problems with manual wrapping.

@jakearchibald
Copy link
Author

The past few editors I've used preserve indenting in their wrapping. I guess I wouldn't be happy with this proposal if my editor didn't do that.

@foolip
Copy link
Member

foolip commented Aug 2, 2017

Looks like https://stackoverflow.com/questions/13559061/emacs-how-to-keep-the-indentation-level-of-a-very-long-wrapped-line would do the trick for Emacs, but if this isn't the default in most editors that people use (Sublime?) then it'll be a bit of a nuisance for newcomers, although easier to explain than our current rules.

@annevk
Copy link
Member

annevk commented Aug 3, 2017

@domenic you're saying wrapping is better for new contributors? They almost always get it wrong in my experience. If we didn't wrap except between paragraphs we'd have far less onboarding issues I think.

@tabatkins
Copy link

tabatkins commented Aug 3, 2017

+1000 for doing almost anything other than "hard-wrap at X columns". It's hard for new contributors, it's annoying to make changes unless your editor comes with "rewrap these lines" functionality, it makes searching very unreliable, it messes up basic diffs... There's really nothing recommending it at all, besides "it's how some other people do it" and "some particularly old editors make it the easiest method".

I definitely prefer semantic wrapping; it feels closest to how code is formatted. Merely hard-wrapping at sentence breaks would also work well. Wrapping purely at the paragraph level is a little annoying personally, but I'm fine with it as well; it's still far better than hard-wrapping at X columns. GitHub didn't used to treat these very good in the viewer (just letting those lines get extremely long); has that changed?

@domenic
Copy link
Member

domenic commented Aug 3, 2017

you're saying wrapping is better for new contributors?

I believe so, or at least, for those used to other software projects. I'm also approaching it from the perspective of someone reading the source in order to edit it, whereas you're approaching it from the perspective of someone submitting a change. For the latter aspect, I'd rather point them toward tooling (which is why I favor machine-enforceable wrapping).

@annevk
Copy link
Member

annevk commented Aug 3, 2017

Yeah, if we had a tool that would help a lot, though needing regular expressions to find terms is still not great.

@foolip
Copy link
Member

foolip commented Aug 4, 2017

A tool that did the wrapping could avoid wrapping inside <span> and <a> and perhaps a few other places.

@jakearchibald
Copy link
Author

Any changes of opinion here? It seems weird that we're still making this human-work.

It would be less bad if specs with hard-wrapping applied it automatically as a git hook/action, but even then I don't really see the benefit.

@annevk
Copy link
Member

annevk commented Aug 6, 2020

@domenic is there tooling we could use? Something like rustfmt, but for Bikeshed and Wattsi input.

@jakearchibald
Copy link
Author

Applying the wrapping automatically at PR/commit time is better, but that still makes find/replace hard. It still feels like wrapping should be a view-time feature.

If diffs are the problem, then maybe a GitHub action could produce a better diff? Or if we have a set of requirements we could take them to GitHub.

@ricea
Copy link

ricea commented Aug 6, 2020

I am in favour of the status quo. If editors and diff tools had soft-wrapping that worked as well as the hard-wrapping does I might change my mind.

@jakearchibald
Copy link
Author

@ricea where are you seeing issues with soft-wrapping? It feels like editors have been handling this pretty well for at least a decade.

@ricea
Copy link

ricea commented Aug 6, 2020

emacs with hard wrapping:

   1. [=Shutdown with an action=] consisting of [=getting a promise to wait for all=] of the actions
      in |actions|, and with |error|.
  1. If |signal|'s [=AbortSignal/aborted flag=] is set, perform |abortAlgorithm| and return
     |promise|.
  1. [=AbortSignal/Add=] |abortAlgorithm| to |signal|.

emacs with soft-wrapping:

   1. [=Shutdown with an action=] consisting of [=getting a promise to wait for all=] of the
actions in |actions|, and with |error|.
  1. If |signal|'s [=AbortSignal/aborted flag=] is set, perform |abortAlgorithm| and return
|promise|.
  1. [=AbortSignal/Add=] |abortAlgorithm| to |signal|.

@jakearchibald
Copy link
Author

In VSCode this would be:

  1. [=Shutdown with an action=] consisting of [=getting a promise to wait for all=] of the actions
  in |actions|, and with |error|.
 1. If |signal|'s [=AbortSignal/aborted flag=] is set, perform |abortAlgorithm| and return
 |promise|.
 1. [=AbortSignal/Add=] |abortAlgorithm| to |signal|.

And it seems like there are ways to have at least that in emacs https://emacs.stackexchange.com/questions/14589/correct-indentation-for-wrapped-lines.

That seems visually good enough, especially with line numbers indicating the start of hard lines. And you don't get any of the find/replace/diff rendering issues.

@ricea
Copy link

ricea commented Aug 6, 2020

I don't use line numbers because they're just noise to me. And w3c/ServiceWorker@65cf285?diff=split looks far worse to me than the equivalent thing with line breaks.

@jakearchibald
Copy link
Author

jakearchibald commented Aug 6, 2020

I disagree. Example:

Screenshot 2020-08-06 at 14 42 38

In the above, the highlighted changes are the actual meaningful changes. Compare to this from the DOM spec:

Screenshot 2020-08-06 at 14 44 53

Here it's a mix of meaningful changes, and wrapping changes.

@domenic
Copy link
Member

domenic commented Aug 6, 2020

@domenic is there tooling we could use? Something like rustfmt, but for Bikeshed and Wattsi input.

I've been using https://domenic.github.io/rewrapper/, which is linked to from the HTML Standard contributing guidelines.

@tabatkins
Copy link

Yeah, the fact that this inarguably makes diffs much nastier, and breaks find-and-replace, makes me still give a strong side-eye to any "better for readability" arguments, or anything based on "my editor doesn't display it in an ideal fashion by default". All reasonable editors can be configured to display soft-wrapping well (and many do that by default, such as Sublime or VSText), and readability of hard-wrapped lines is a very subjective argument with differing opinions. (I personally find it harder to read, because I'm trained now to see lines broken at anything other than the page edge to be semantically-wrapped; if the break is just arbitrary, it confuses me for a moment.)

I really want us to evaluate this without a status-quo bias here. If you were generating these documents fresh, given what we know of the usability issues it causes, would we have imposed a hard-wrap requirement?

I definitely prefer semantic wrapping; it feels closest to how code is formatted. Merely hard-wrapping at sentence breaks would also work well. Wrapping purely at the paragraph level is a little annoying personally, but I'm fine with it as well; it's still far better than hard-wrapping at X columns.

Revisiting this... while I'd still be okay with paragraph-level breaking (it's still better than a column-based limit), it's still a relatively annoying method, as it means any changes will show up in diffs as a whole-paragraph change. At bare minimum, breaking on sentence boundaries would work better, and it's unambiguous (as opposed to strong semantic wrapping like I do, which does rely on some judgement about where to break phrases apart).

@jakearchibald
Copy link
Author

as it means any changes will show up in diffs as a whole-paragraph change

This doesn't seem that big a deal given that GitHub also highlights changes within a paragraph.

@tabatkins
Copy link

Yeah, GH reduces the issue, but it's still more text highlighted in the line-diff styling than would be otherwise necessary. And when doing git on your own, you don't get that unless you're telling it to do a word diff.

@jakearchibald
Copy link
Author

https://github.com/w3c/screen-wake-lock/pull/275/files this PR adds one letter and a space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants