-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Chore: enable max-len. #9414
Chore: enable max-len. #9414
Conversation
LGTM |
LGTM |
LGTM |
Oh, could we please not enable this rule. I have it enabled on my code-base, and I absolutely hate it. I don't see the value in limiting the length of the string, but it creates a huge number of formatting inconsistencies the developers need to spend time fixing. |
@ilyavolodin This PR enables the ignoreStrings option (and pretty much all ignore options). So strings don't have to be artificially broken up (unless the line is already too long even without the string, and in that case the string can go down a line). Am I missing something? |
|
I don't really feel strongly about enabling If the |
Deeply nested code also probably could/should be refactored into functions. 😀 But I don't strongly mind whether we turn this on or not. |
I just know that I've had max-len enabled on my work project for a year now, and I finally gave up and disabled it few days ago, cause I couldn't force myself to constantly correct my code when it's perfectly readable as is. So it's a personal thing. As I said, if everyone else fills like we should enable it, I'll adjust (I've been doing it for over a year, so it's inconvenient but fine). |
haha, I had the similar experience. so the config is as tolerant as possible. |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Other, please explain:
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
since we are using 4 spaces indent, it's very easy to exceed 120, I set max-len to 160 and ignore options to true.