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

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

Conversation

@vmk1vmk
Copy link
Contributor

commented Jul 13, 2019

PR checklist

  • Addresses an existing issue: fixes #3905
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

Failing in testNext because of #4784

Copy link
Collaborator

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

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
Collaborator

left a comment

Great! Thanks for sending this in @vmk1vmk! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit b297249 into palantir:master Jul 16, 2019
13 of 14 checks passed
13 of 14 checks passed
ci/circleci: testNext Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: checkout-code Your tests passed on CircleCI!
Details
ci/circleci: clean-lockfile Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test2.1 Your tests passed on CircleCI!
Details
ci/circleci: test2.4 Your tests passed on CircleCI!
Details
ci/circleci: test2.7 Your tests passed on CircleCI!
Details
ci/circleci: test2.8 Your tests passed on CircleCI!
Details
ci/circleci: test2.9 Your tests passed on CircleCI!
Details
ci/circleci: test3.0 Your tests passed on CircleCI!
Details
ci/circleci: testRc Your tests passed on CircleCI!
Details
cla/palantir CLA signed on 2019-03-22 19:49 UTC+00:00
Details
@vmk1vmk vmk1vmk deleted the vmk1vmk:feature/ignore-strings-in-max-line-length branch Jul 16, 2019
@adidahiya adidahiya referenced this pull request Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.