Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[max-line-length] ignore strings and regex in max line length #4798

Conversation

vmk1vmk
Copy link
Contributor

@vmk1vmk vmk1vmk commented Jul 13, 2019

PR checklist

Overview of change:

Added functionality to ignore strings exceeding the max line length, it is by default enabled.
Added option check-strings to control if exceeding strings are skipped or treated as errors.
Added the same option for regex called check-regex.

Is there anything you'd like reviewers to focus on?

  • Are all "string"-cases covered?
  • Does this new functionality behave as expected?
  • Should it be split into different options like check-plain-strings and check-template-strings?

CHANGELOG.md entry:

[enhancement] Added check-strings and check-regex options to max-line-lenght rule.

@vmk1vmk vmk1vmk changed the title Feature/ignore strings in max line length [max-line-length] ignore strings and regex in max line length Jul 14, 2019
@vmk1vmk
Copy link
Contributor Author

vmk1vmk commented Jul 14, 2019

Failing in testNext because of #4784

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM, thanks for sending this in! A few requested changes around readability.

src/rules/maxLineLengthRule.ts Outdated Show resolved Hide resolved
src/rules/maxLineLengthRule.ts Outdated Show resolved Hide resolved
src/rules/maxLineLengthRule.ts Show resolved Hide resolved
… combined the two consecutive filter calls into one.
@vmk1vmk
Copy link
Contributor Author

vmk1vmk commented Jul 16, 2019

@JoshuaKGoldberg Thanks for reviewing so fast. I made all requested changes.
How about the functionality and the options? Do you think they should be more granular like check-strings and check-template-strings?

@JoshuaKGoldberg
Copy link
Contributor

How about the functionality and the options? Do you think they should be more granular like check-strings and check-template-strings?

No, this looks fine. TSLint's being deprecated soon (#4534) and has been recommending using Prettier instead of formatting rules for quite some time. I don't think that extra logic will be worth the added complexity.

Are all "string"-cases covered?
Does this new functionality behave as expected?

Looks good! 😄 I played around with this locally a bit and it worked great.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks for sending this in @vmk1vmk! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit b297249 into palantir:master Jul 16, 2019
@vmk1vmk vmk1vmk deleted the feature/ignore-strings-in-max-line-length branch July 16, 2019 23:30
crisbeto added a commit to crisbeto/material2 that referenced this pull request Nov 1, 2019
In tslint 5.19.0 they added a separate option that determines whether strings should be checked and it seems to have been defaulted to `false` (palantir/tslint#4798). As a result we ended up with a few places where the limit was being violated.

These changes explicitly enable checking of strings and fix the failures that we had accumulated.
jelbourn pushed a commit to angular/components that referenced this pull request Nov 1, 2019
In tslint 5.19.0 they added a separate option that determines whether strings should be checked and it seems to have been defaulted to `false` (palantir/tslint#4798). As a result we ended up with a few places where the limit was being violated.

These changes explicitly enable checking of strings and fix the failures that we had accumulated.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max-line-length with template literals
2 participants