eslint-plugin-unicorn icon indicating copy to clipboard operation
eslint-plugin-unicorn copied to clipboard

Rule proposal: `wrap-comments`

Open mmkal opened this issue 3 years ago • 21 comments

I spend too much of my life "balancing" jsdoc and // comments so they're roughly the same length as each other. The consequences of doing this/not doing this are:

  • [doing] precious time spent doing busywork
  • [not doing] single line comments that are wider than most screens
  • [not doing] comments that have weird line breaks and are less readable
  • [doing] comments that don't get updated because they've already been meticulously aligned - so adding or deleting words would screw up the alignment

This is something humans are bad at and machines are good at. The rule could take the comment block, tokenize it and apply line breaks at the closest point to the target line length based on natural word breaks. The algorithm doesn't need to be complex. str.split(' ') is probably good enough for a first cut (/until there are complaints).

Of course this should be auto-fixable, otherwise it would be the worst thing ever.

Fail

/** a very very very very very very very very very very very very very very very very very very very very very very very very long comment */
foo();
// a short comment
// that should be on one line
foo();

Pass

/**
 * a very very very very very very very very very very very very very very very
 * very very very very very very very very very long comment
 */
foo();
// a short comment that should be on one line
foo();

Configuration

This rule should probably be fairly aggressive by default, but for the sake of adoption would need configuration:

type Config  = {
  maxLineLength: number; // default 80
  allowShortSlashSlashComments: 'end-of-sentence' | 'never' | 'always' // default 'end-of-sentence'
}
// Error, comment lines too short:

/* eslint "unicorn/wrap-comments": ["error", {"allowShortSlashSlashComments": "never"] */
// foo!
// bar.
// baz?
// qux
foo();

// Ok, comment lines are short but separated by whitespace

// foo

// bar
foo();

// Ok, comment lines are short but end with sentence punctuation:

/* eslint "unicorn/wrap-comments": ["error", {"allowShortSlashSlashComments": "end-of-sentence"}] */
// foo!
// bar.
// baz?
// qux
foo();

I'd be happy to implement this rule but it's so generic that I think it belongs in a community library like this.

c.f. this prettier issue which is almost done with preschool: https://github.com/prettier/prettier/issues/265

mmkal avatar Aug 06 '21 15:08 mmkal

Why not just use soft-wrapping? Meaning, let the editor handle the wrapping. I honestly never understood why people prefer hard-wrapping. It's just a lot of manual work that has to happen every time there are changes.

sindresorhus avatar Aug 06 '21 21:08 sindresorhus

How should it handle things that cannot be broken, like long URLs?

sindresorhus avatar Aug 06 '21 21:08 sindresorhus

Re soft-wrapping: I do use it, but sometimes it can hurt the readability of the code, so I usually have it off, and turn it on in a pinch. Plus, while developers might spend most of their time in [IDE of choice], there are a lot of places that code appears - including in various GitHub UIs, like its diff view. There's GitHub's standard code viewer, diff viewer, raw viewer, and all of the above at various window width (including mobile - which is a legit way of reviewing code, if not writing it). Plus every viewer on GitLab, AWS Cloud9, BitBucket, Azure Repos, Gitpod, etc. etc. etc. I have found myself looking at old code on Dropbox on my Android from time to time.

As an example based on what's in my IDE right now: I am currently using my 15-inch laptop with VSCode. I'm using a split pane, with one sql file on the right, and a typescript file on the left. With soft wrapping it looks like this:

image

It's just... confusing. There's definitely a use for soft-wrapping, but it's not a full solution because it does unwanted things a significant proportion of the time.

It's just a lot of manual work that has to happen every time there are changes

That's basically what I hope this rule will fix.

How should it handle things that cannot be broken, like long URLs?

If they're longer than maxLineLength, just let them run over the line length. We could borrow from prettier's philosophy here. We'd need a few heuristics/rules for when we do and don't apply line breaks:

printWidth option does not work the same way. It is not the hard upper allowed line length limit. It is a way to say to Prettier roughly how long you’d like lines to be. Prettier will make both shorter and longer lines, but generally strive to meet the specified printWidth.

This rule would never be perfect. But I think without too much effort it could make virtually everyone happy 95% of the time. The people it doesn't make that happy can keep fiddling manually, or help improve the heuristics.

mmkal avatar Aug 06 '21 21:08 mmkal

@mmkal unfortunately your screenshot highlights exactly why hard wrapping should not be used. If you didn't use hard wrapping, it would have just fit your window perfectly:

128573501-d4bc17a7-9d96-435d-b367-3afc28638f2d

This is the exact argument that people use against soft wrapping without realizing that they're actually using both. Discussion example: https://github.com/sindresorhus/refined-github/issues/3112#issuecomment-631482193

fregante avatar Aug 11 '21 17:08 fregante

@fregante sorry, I should have been clearer about the issue I was trying to communicate with that screenshot. I'm not so worried about the comments - it's the code that's the problem, and the reason I have soft-wrapping turned off almost all the time.

It's the way lines 31 and 35 run over and make the important part hard to parse. That is very often worse than just letting the end of the line go beyond the window (in general, ends-of-lines are less important than starts-of-lines).

I do get what you're saying, but this comment is not readable by anyone who isn't using refined-github (comment generated by OpenAI):

// It is almost always better to include line breaks in comments because it makes the comment more readable and easier to understand. It also makes it easier to separate the comment from the code that it is describing.
const foo = 1

So while on some teams, or for some individuals, refined-github is an OK requirement, it isn't for all. And it doesn't cover the other places code can appear where hard-wrapped comments are better, or soft-wrapping isn't an option.

To be honest, I don't expect to be able to convince anyone, since it's possible to create workflows around either setup. There are also perfectly ok arguments for both tabs and spaces, and vi and emacs. I just think this would be a reasonable rule for this plugin to include, even if not every user wants to turn it on.

mmkal avatar Aug 11 '21 21:08 mmkal

XO/Unicorn doesn't care for line lengths at the moment. Want to have a 1000-char JSX tag? Go ahead. Why should comments be any different?

What it tries to enforce though is "one operation per line". Could this be translated to "one sentence per comment line"? i.e. break long lines at a period. This is what I usually do to avoid super-long lines.

Ideally it's really up to the visualizer to soft-wrap comments. This is what WebStorm does in some cases for example: (Line 10 is soft-wrapped at 80 char; Line 12 is in "editing mode", which isn't wrapped)

Screen Shot 16

Overall I'd be in favor of adding it as a warning on 200+ char-long lines (on comments and on code) and to possibly suggest to wrap at periods when that happens. Hard-wrapping in the middle of a sentence though not so much, because that impacts soft-wrapping users. URLs and other unbreakable strings would be exempt, as long as they're on their own line.

fregante avatar Aug 17 '21 06:08 fregante

XO/Unicorn doesn't care for line lengths at the moment

XO will never error or warn on too long comments. I prefer soft-wrapping.

sindresorhus avatar Aug 17 '21 10:08 sindresorhus

I agree with @mmkal

This rule would never be perfect. But I think without too much effort it could make virtually everyone happy 95% of the time. The people it doesn't make that happy can keep fiddling manually, or help improve the heuristics.

I'd like this rule as well. No problem keeping it out of the recommended preset.

However, that said, what I'd really like is for the IDE to have soft-wrapping for comments only. Now that would be great. I also hate soft-wrapped code, in general.

By the way, @mmkal, I found an IDE configuration that made it "less worse" (to me):

image

image

papb avatar Jan 15 '22 03:01 papb

A thought - @sindresorhus if you prefer soft wrapping, all we’d need to do is create the rule as described and set targetWidth: Infinity in the recommended config. Then this plugin would actually enforce that preference.

People like me who prefer hard-wrapping would override with targetWidth: 100 or whatever. We’d both get what we want and it would never need to be talked about by humans again. What do you think?

mmkal avatar Jan 15 '22 13:01 mmkal

@mmkal What if you have two comments? How would it differentiate that? I wouldn't want these to be forced into a single line.

// TODO: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
// This is describing how the function works. Consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
function foo() {}

sindresorhus avatar Jan 15 '22 18:01 sindresorhus

I’m of course open to debate on this, but maybe by trying to make those two comments into one, the rule could incentivise arguably cleaner code:

option 1, and probably my preferred: encourage the user to make the second comment a jsdoc comment, so that it shows up on hover at callsites in IDEs:

// TODO: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
/** This is describing how the function works. Consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. */
function foo() {}

option 2, necessary in some cases where you really do want two separate // comments, and not jsdoc: force you to separate them with a newline to make the separation clear and visible:

// TODO: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

// This is describing how the function works. Consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
function foo() {}

Having said that, maybe there could be some kind of lightweight way of signaling that a line break should always appear somewhere (off the top of my head, it could work similarly to how vscode renders jsdoc - as markdown, so single newlines are ignored, but double newlines are respected. But lines starting with - are considered bullets so are respected too).


Edit: alternative if you don't agree that the above workarounds are actually improvements: have a config param commentTypes which is set to ['/*'] in the eslint-plugin-unicorn recommended config. Extremists like me could set it to ['/*', '//'].

mmkal avatar Jan 15 '22 22:01 mmkal

None of these options are something I would want.

The one solution I would use is that it would enforce a single line if the first line does not end in . and the next line does not start with a capitalized word (for example, Foo).

Example:

// Lorem ipsum dolor sit amet, consectetur
// adipiscing elit, sed do eiusmod tempor.

This is non-ambigious and should be enforced to a single line.

sindresorhus avatar Jan 16 '22 06:01 sindresorhus

That would work for me too. Probably the line break markers should be ['.', '!', '?', ':'] and not just ..

Ok, sounds like it would be considered if I opened a PR? We could at least see how it looks when the autofix runs against this repo?

mmkal avatar Jan 16 '22 14:01 mmkal

That would work for me too. Probably the line break markers should be ['.', '!', '?', ':'] and not just ..

👍

Ok, sounds like it would be considered if I opened a PR? We could at least see how it looks when the autofix runs against this repo?

👍

sindresorhus avatar Jan 16 '22 15:01 sindresorhus

Tried it. I've got a basic rule working, which is good for simple cases, but on trying it out on this repo and my team’s at work, I hadn't thought of temporarily-commented-out code, weird markdown in jsdoc, as well as many different way that humans organically use conventions to break lines, e.g. with capitals

// This is a line
// This is another line

Or format things very deliberately:

// foo bar
//     ^^ pointing at bar

So I'm putting this on hold for now. If anyone feels like trying this out, let me know and I can share what I have so far. In the meantime I'll try to think of a good heuristic for determining "this is a paragraph of prose and it's safe to fiddle with the line breaks" vs "not sure what this, I'm going to leave it alone" that is simple and doesn't involve writing a general natural language parser.

mmkal avatar Jan 20 '22 13:01 mmkal

A simple method could be to look for consecutive special characters, i.e. characters that are not used to write a comment and are not alphanumeric. E.g. do not format the comment if the following regex matches:

/([^\p{L}0-9\/* ])((?=\1)|(?!\1))([^\p{L}0-9\/* ])/gu

devgioele avatar May 22 '22 14:05 devgioele

Just hopping in from eslint ticket. Like I said there: how about just have the option to enforce line breaks in jsdoc comments and not // or //? I don't like the line breaks in // comments especially ans sindre mentioned when its a done/todo/... thing. As well as for // descriptive stuff

muuvmuuv avatar Sep 30 '23 17:09 muuvmuuv