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

chore(*): correctly scope disabled max-line-length tslint rule #2308

Merged
merged 1 commit into from
Feb 14, 2017
Merged

chore(*): correctly scope disabled max-line-length tslint rule #2308

merged 1 commit into from
Feb 14, 2017

Conversation

gkalpak
Copy link
Contributor

@gkalpak gkalpak commented Jan 23, 2017

Description:
The max line length is set to 150 in 'tslint.json'. In specific regions, it is desirable to allow longer lines, so these regions should be wrapped in comments like the following:

// Max line length enforced here.

/* tslint:disable:max-line-length */
// Max line length NOT enforced here.
/* tslint:enable:max-line-length */   <-- CORRECT

// Max line length enforced here.

In many cases, the re-enabling comment incorrectly included disable instead of enable (as shown below), which essentially keeps the max-line-length rule disabled for the rest of the file:

// Max line length enforced here.

/* tslint:disable:max-line-length */
// Max line length NOT enforced here.
/* tslint:disable:max-line-length */   <-- INCORRECT

// Max line length NOT enforced here.

This commit fixes these comments, so the max-line-length rule is properly enforced in regions that don't need longer lines.

(Luckily, there was only 1 violation 😃)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0003%) to 97.698% when pulling 4becdd0 on gkalpak:patch-5 into 31cf2bf on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Jan 29, 2017

@gkalpak can you better outline the issue that this fixes?

@gkalpak gkalpak changed the title chore(*): fix incorrect tslint comments chore(*): correctly scope disabled max-line-length tslint rule Jan 29, 2017
The max line length is set to 150 in 'tslint.json'. In specific regions, it is
desirable to allow longer lines, so these regions should be wrapped in comments
like the following:

```js
// Max line length enforced here.

/* tslint:disable:max-line-length */
// Max line length NOT enforced here.
/* tslint:enable:max-line-length */   <-- CORRECT

// Max line length enforced here.
```

In many cases, the re-enabling comment incorrectly included `disable` instead of
`enable` (as shown below), which essentially keeps the `max-line-length`
**disabled** for the rest of the file:

```js
// Max line length enforced here.

/* tslint:disable:max-line-length */
// Max line length NOT enforced here.
/* tslint:disable:max-line-length */   <-- INCORRECT

// Max line length NOT enforced here.
```

This commit fixes these comments, so the `max-line-length` rule is properly
enforced in regions that don't need longer lines.
@gkalpak
Copy link
Contributor Author

gkalpak commented Jan 29, 2017

@Blesh, I have updated the commit message and PR description.

@david-driscoll
Copy link
Member

@Blesh looks like a goof on my part, this change looks good from that perspective.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0003%) to 97.702% when pulling 37b6e39 on gkalpak:patch-5 into d4533c4 on ReactiveX:master.

@coveralls
Copy link

coveralls commented Jan 30, 2017

Coverage Status

Coverage increased (+0.0003%) to 97.702% when pulling 37b6e39 on gkalpak:patch-5 into d4533c4 on ReactiveX:master.

Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

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

makes sense to me.

@jayphelps
Copy link
Member

jayphelps commented Feb 14, 2017

Yeah, this is a whoopies! Thank you much!!

@jayphelps jayphelps merged commit 25ee5b3 into ReactiveX:master Feb 14, 2017
@gkalpak gkalpak deleted the patch-5 branch February 15, 2017 11:25
@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
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.

6 participants