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

Exceeding maximum line length of 120 characters #23

Closed
ilesar opened this issue Mar 19, 2018 · 12 comments
Closed

Exceeding maximum line length of 120 characters #23

ilesar opened this issue Mar 19, 2018 · 12 comments
Assignees

Comments

@ilesar
Copy link

ilesar commented Mar 19, 2018

I think we should be less strict about this rule. Just to prove my point, I'm attaching a screenshot with proper class declaration where TSLint gives me an error. Maybe the class names are a bit long, but they they are named so code readability wouldn't suffer.

image

If we vote this through, I'll make a pull request.

@krukru
Copy link
Contributor

krukru commented Mar 19, 2018

I like how the rule is stated for PHP (PSR-2)
There MUST NOT be a hard limit on line length; the soft limit MUST be 120 characters; lines SHOULD be 80 characters or less.
We can apply the same rule to JS

@bonzzy
Copy link
Contributor

bonzzy commented Mar 20, 2018

Tslint supports ignore patterns, so i suggest we could add a few exclude patterns instead.
For example
"max-line-length": [true, {"limit": 120, "ignore-pattern": "^import |^export {(.*?)}"}]

@krukru
Copy link
Contributor

krukru commented Mar 20, 2018

I would not like to have exceptions.
The question is how bad is 1) compared to 2)

export class SnapshotResolver extends BaseResolver<PatchableOrderBook> implements ResolvableInterface<PatchableOrderBook> {
    const foo = this.bar();
} 
export class SnapshotResolver extends BaseResolver<PatchableOrderBook>
        implements ResolvableInterface<PatchableOrderBook> {
    const foo = this.bar();
} 

I would go with 2) for readability but we can bend the rule if you think it is too strict

@bonzzy
Copy link
Contributor

bonzzy commented Mar 20, 2018

I think we should avoid very long lines. But that is only my personal opinion.

I think 2) is easier to read.

@leovujanic
Copy link
Contributor

What about tests and their descriptive names or long Error messages?

@krukru
Copy link
Contributor

krukru commented Mar 30, 2018 via email

@leovujanic
Copy link
Contributor

Yes, but code visibility is defined by screen size, font size etc. Also in the editor, you can set up to use a soft wrap option.
I think that lines should not be longer than 120 chars but that should be only the soft limit because I do not want to change this

throw new Error(`Invalid precision. Precision must be 0 or positive number less than ${maxSafeString.length - 1}`);

to this:

throw new Error(`Invalid precision. Precision must be 0 or positive number 
less than ${maxSafeString.length - 1}`);

or this:

const msg = `Invalid precision. Precision must be 0 or positive 
number less than ${maxSafeString.length - 1}`;
throw new Error(msg);

Sometimes I think we are making idiots from people because there should be a minimum dose of responsibility and behavior manners to respect the rule and not to write everything in one line only in exceptional cases.

@krukru
Copy link
Contributor

krukru commented Mar 30, 2018

@leovujanic I am sorry if you think someone is making a fool of people. The idea is to have a consistent code style that is readable from any platform (github included) and not to make a fool of anyone.
Detecting when is it ok to have more then 120 characters is a difficult problem for humans, and even harder for linters. This is why we are either always strict or always permissive.

We could modify the rule to ignore strings, multiline broken strings are difficult to search and some style guides with fixed line length allow for long strings.

If you can write the rule for it in tslint, we can discuss about including it

@leovujanic
Copy link
Contributor

No, I don't think that someone is making a fool of people intentionally, but I don't understand why can't we just say "Please keep your lines under 120 chars". I see a lot of /* tslint:disable:.... */ comments in code so maybe that linter is too strict. What would be the solution for implementing advice from PSR standard that you mentioned before ( #23 (comment))?

@bonzzy
Copy link
Contributor

bonzzy commented Apr 3, 2018

Check possible solution to this problem on degordian/js-coding-standards/issues/26

@bonzzy
Copy link
Contributor

bonzzy commented Apr 25, 2018

Can we close this?

@krukru
Copy link
Contributor

krukru commented May 16, 2018

I think that we did not get enough strong opinions that this should be enforced as a hard limit.
My advice is to keep it within 120 characters wide, but we will not reject PRs with lines that are longer.

@krukru krukru closed this as completed May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants