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.

Sign up for GitHub

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

Jump to bottom

Fix: Add class options for lines-around-comment (fixes #8564) #8565

Merged
merged 1 commit into from Oct 14, 2017
Merged

Fix: Add class options for lines-around-comment (fixes #8564) #8565

merged 1 commit into from Oct 14, 2017

Conversation

Mardak
Copy link
Contributor

@Mardak Mardak commented May 8, 2017 •
edited

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fix #8564.

What changes did you make? (Give an overview)
Moved the "ClassBody" check from "Block" to "Object" Added allowClassStart and allowClassEnd options, updated tests, and added docs.

Is there anything you'd like reviewers to focus on?
Should this be marked breaking?

@jsf-clabot
Copy link

jsf-clabot commented May 8, 2017 •
edited

CLA assistant check
All committers have signed the CLA.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@Mardak, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mathieumg, @scriptdaemon and @alberto to be potential reviewers.

@not-an-aardvark not-an-aardvark added breaking This change is backwards-incompatible enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels May 14, 2017
@eslintbot
Copy link

LGTM

@Mardak Mardak changed the title Breaking: Switch class option for lines-around-comment (fixes #8564) Breaking: Add class options for lines-around-comment (fixes #8564) May 17, 2017
@eslintbot
Copy link

LGTM

@Mardak Mardak changed the title Breaking: Add class options for lines-around-comment (fixes #8564) Fix: Add class options for lines-around-comment (fixes #8564) May 21, 2017
@Mardak
Copy link
Contributor Author

Mardak commented May 21, 2017

This PR should no longer be a breaking rule change as indicated by the old tag.

@eslintbot
Copy link

LGTM

@Mardak Mardak mentioned this pull request Jun 27, 2017
Closed
@not-an-aardvark not-an-aardvark removed the breaking This change is backwards-incompatible label Jun 27, 2017
ilyavolodin
ilyavolodin suggested changes Jun 27, 2017
View reviewed changes
docs/rules/lines-around-comment.md Outdated
@@ -172,6 +172,55 @@ function foo(){
}
```

### allowClassStart

Examples of **correct** code for this rule with the `{ "beforeLineComment": true, "allowClassStart": true }` option:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a few incorrect code examples too. For example, { "beforeLineComment": true, "allowClassStart": false }, or { "beforeLineComment": false, "allowClassStart": true }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added incorrect examples with allowClassStart: false. It's not as relevant for incorrect examples with allowClassStart: true as it's making the check less strict.

@eslintbot
Copy link

LGTM

@Mardak
Copy link
Contributor Author

Mardak commented Aug 7, 2017 •
edited

Do the labels/tags on this approved PR need to be adjusted?

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 13, 2017
@kaicataldo
Copy link
Member

This has now been accepted. Apologies for the delay!

kaicataldo
kaicataldo reviewed Oct 13, 2017
View reviewed changes
docs/rules/lines-around-comment.md Outdated

class foo {
// what a great and wonderful day
day() {"great"}
Copy link
Member

Choose a reason for hiding this comment

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

Can we all these examples say day() {} or day() { return great; } instead? It's a bit confusing as it currently stands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched them all to day() {} (and rebased)

@eslintbot
Copy link

LGTM

@kaicataldo kaicataldo merged commit 62a323c into eslint:master Oct 14, 2017
@kaicataldo
Copy link
Member

LGTM. Thank for contributing to ESLint!

@mysticatea
Copy link
Member

Just FYI, the release note doesn't seem to list this PR in the Enhancements section.

@kaicataldo
Copy link
Member

kaicataldo commented Oct 16, 2017 •
edited

This was my fault. I missed that the commit message said Fix at the beginning... I'll move it to the enhancements section for the blog post.

@kaicataldo
Copy link
Member

Relevant comment: #8633 (comment)

@kaicataldo kaicataldo mentioned this pull request Oct 16, 2017
Closed
kaicataldo added a commit to eslint/archive-website that referenced this pull request Oct 16, 2017
@kaicataldo
Update 2017-10-14-eslint-v4.9.0-released.md
88b6985 
Fixes eslint/eslint#8565 (comment)
@Mardak Mardak deleted the issue8564 branch October 16, 2017 15:03
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 13, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ilyavolodin ilyavolodin ilyavolodin approved these changes

@kaicataldo kaicataldo kaicataldo approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lines-around-comment ES2015 classes in new allowClassStart option
8 participants
@Mardak @jsf-clabot @eslintbot @mention-bot @kaicataldo @mysticatea @ilyavolodin @not-an-aardvark