sindresorhus / eslint-plugin-unicorn

More than 100 powerful ESLint rules
MIT License
4.25k stars 367 forks source link

Rule proposal: `wrap-comments` #1467

Open mmkal opened 3 years ago

mmkal commented 3 years ago

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:

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

sindresorhus commented 3 years ago

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 commented 3 years ago

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

mmkal commented 3 years ago

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.

fregante commented 3 years ago

@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

mmkal commented 3 years ago

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

fregante commented 3 years ago

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.

sindresorhus commented 3 years ago

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.

papb commented 2 years ago

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

mmkal commented 2 years ago

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?

sindresorhus commented 2 years ago

@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() {}
mmkal commented 2 years ago

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 ['/*', '//'].

sindresorhus commented 2 years ago

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.

mmkal commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

devgioele commented 2 years ago

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
muuvmuuv commented 1 year ago

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

mmkal commented 2 months ago

@muuvmuuv that's a good idea, although I'm not sure what eslint ticket you're referring to. JSDoc comments can more reasonably be expected to be prose, and any part of them that's code should use @example markers and/or ``` blocks.

karlhorky commented 2 months ago

IMO wrapping // comment lines by default is good, as long as certain patterns aren't met. // comments are a common way to write blocks of prose in a lot of codebases. It's also the default comments when using the multi-line comment hotkey in default VS Code (and other editor) configuration.

Eg. these two lines from Sindre's comment should be hard wrapped by default to multiple // lines (depending on configured line length), because there is no clear pattern to prevent it:

// 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() {}

To prevent wrapping on a // line, some options:

  1. Add an extra empty // line in between, creating a new paragraph
  2. Some "smart" heuristic to prevent the wrapping (potentially also with ability to disable), although capitals alone seem like not enough (capital letters may be required in the middle of a line which should be wrapped)
  3. Use a bulleted or numbered list to start a new line (although prose contained in the list items should be wrapped to multiple lines, in the same formatting style to Markdown lists)
  4. Use ``` to make non-wrapped blocks of code
  5. Use the option to disable wrapping on all // comment lines

While I do think // comments should be wrapped to multiple lines, I could also imagine an option for those wanting them on a single line, as originally proposed.

karlhorky commented 2 months ago

Prior art here is the Rewrap VS Code extension (F#, C#), which does many things well, including formatting of numbered lists and ignoring code blocks:

jaydenseric commented 2 months ago

eslint-plugin-comment-length works pretty good; we've been using it for some time with ESLint v8. We've had to temporarily drop it in projects we have migrated to ESLint v9 though, until it's supported:

https://github.com/lasselupe33/eslint-plugin-comment-length/issues/10