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

meta: Are the limits on line length still necessary? #14176

Closed
refack opened this issue Jul 11, 2017 · 16 comments
Closed

meta: Are the limits on line length still necessary? #14176

refack opened this issue Jul 11, 2017 · 16 comments
Labels
discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project.

Comments

@refack
Copy link
Contributor

refack commented Jul 11, 2017

  1. 50 chars for commit title
  2. 72 chars for commit message
  3. 80 chars for code line

I wanted to start a discussion on why we're still enforcing them? Is it possible to extend the limits?
Especially the 80 char per code line. There are style guides that extend the limit to 120 chars, or make an exception for function calls with many arguments, and string literals.
I'm assuming originally it came from narrow terminal windows, and that it's been kept popular under the assumption that one line of code should not do too much. But sometimes it leads to very cumbersome constructs:
a random example taken from ./configure (that is actually a file that is not auto-linted.)

parser.add_option('--with-mips-arch-variant',
    action='store',
    dest='mips_arch_variant',
    default='r2',
    choices=valid_mips_arch,
    help='MIPS arch variant ({0}) [default: %default]'.format(
        ', '.join(valid_mips_arch)))

while lines that do multiple operations are considered ok

sys.exit(subprocess.call(gyp_args))

Another example is documentation files, where we don't auto-lint so it's easy to find examples where it was not upheld. Also, AFAIK most editors can soft-wrap lines while files are anyway rendered to something prettier.

Ref: #8327

@refack refack added discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project. labels Jul 11, 2017
@vsemozhetbyt
Copy link
Contributor

Refs (to compare with other popular guides): Airbnb

@refack refack mentioned this issue Jul 11, 2017
2 tasks
@refack
Copy link
Contributor Author

refack commented Jul 11, 2017

@gibfahn
Copy link
Member

gibfahn commented Jul 11, 2017

I have found this more and more irritating in code. If your editor wraps then I'm not sure what the benefit is. I'd be +1 on increasing it to 100/120.

For git I think it's different, the 50 char limit encourages people to be concise in the title, which is really helpful when you're doing a git log --oneline. GitHub wraps the title at 72 chars, so we couldn't go beyond that anyway:

image

For the git body (and for .md files, anything that's flowing text really) I think the limit is helpful when reading it unformatted. If you google "best line length for readability" the answers are usually around 72 chars. I think we already don't apply that to URLs of course, so maybe being more flexible in what we care about is the key.

I like the Airbnb style @vsemozhetbyt mentioned, ignoring strings and URLs (but not comments) makes sense:

'max-len': ['error', 100, 2, {
  ignoreUrls: true,
  ignoreComments: false,
  ignoreRegExpLiterals: true,
  ignoreStrings: true,
  ignoreTemplateLiterals: true,
  }],

@sam-github
Copy link
Contributor

re: git, http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

I don't agree that we have more screen space now, because once the screen got wider, it became possible to put up two tabs side-by-side, which is what I do, and assume most people do, and both are 90 chars wide, as it happens, on my monitor, leaving room if I wanted it for some kindof tool tray to the side.

IMO, most examples of long lines are either poorly written code, or because of long argument lists. With multi-arg functions and long descriptive names, there will always be wrapping, might as well wrap early (and hope people write functions with less args).

Its also hard to follow lines longer than 70 or 80 chars visually, which is why newspapers format text into short columns.

And diff tools do well with inter-line and not so well with inside-the-line text.

Etc, etc.

@gibfahn
Copy link
Member

gibfahn commented Jul 11, 2017

On the "annoying for those who don't have a wide screen" thing: Do you fall into that category?

This is my normal setup:

skaermbillede 2017-07-12 kl 00 08 28

I'm sure there are ways I could get 10 chars extra but I don't think that is what we are talking about.


I don't mind 80+ lines when it is clear that no better alternative exists (such as URLs). However, in this case, I strongly believe better alternatives exist.

@AndreasMadsen (from #14173 (comment)), have you tried line wrapping? I often use a narrow editor, and I find the wrapping to be cleaner than what we do now:

image

Not saying wrapping fixes all problems obviously, just a suggestion.

@evanlucas
Copy link
Contributor

I'd be +1 on extending the commit title to 72 columns, but prefer we keep the commit and code line lengths the same. I find that auto wrapping makes it more difficult to immediately see things the way sticking to a smaller line length does

@mscdex
Copy link
Contributor

mscdex commented Jul 11, 2017

I prefer the limits the way they are now.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 12, 2017

I prefer the current limits as well.

@XadillaX
Copy link
Contributor

Agree with 120 columns per code line.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 12, 2017

have you tried line wrapping? I often use a narrow editor, and I find the wrapping to be cleaner than what we do now:

skaermbillede 2017-07-12 kl 07 45 19

It looks like this. Honestly, I can't stand it and would rather scroll. Also, with an argument like that why have a limit at all.

@bnoordhuis
Copy link
Member

I also prefer the current line length. Why, I feel 80 columns is already overly indulgent. When I was young we only had 72 columns and we were happy, we didn't know any better.

@XadillaX
Copy link
Contributor

In my VIM, I have an 80-column warning line and a 120-column error line. That means I usually keep it's length up to 80 and if necessary, it still can't longer than 120.

image

@tniessen
Copy link
Member

  1. The 50 char limit of the title is somewhat restrictive, I am ±0 on this, but I don't have a problem with keeping it as it is.
  2. 72 chars should be kept as they are IMO, that barely ever causes problems.
  3. I think lines with more than 100 characters are barely readable. The only reason to have such long lines is to include string literals etc., see Ignore template literals #14173. Everything else can comfortably be wrapped in JavaScript.

@MylesBorins
Copy link
Contributor

The 50 / 72 rule has historic president and many tools are built around them

I am -1 to changing our commit guidelines

@mhdawson
Copy link
Member

I'm +1 to leaving as it is.

@bnoordhuis
Copy link
Member

It's clear there is no consensus on making changes. I'll go ahead and close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests