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

New: new rule no-constructor-return (fixes #12481) #12529

Merged
merged 13 commits into from Nov 21, 2019
Merged

New: new rule no-constructor-return (fixes #12481) #12529

merged 13 commits into from Nov 21, 2019

Conversation

@g-plane
Copy link
Member

g-plane commented Nov 5, 2019

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

New rule

What changes did you make? (Give an overview)
Added new rule: no-constructor-return

Is there anything you'd like reviewers to focus on?
Are there anything should be polished at the documentation?
Are there more cases to be handled?

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented on docs/rules/no-constructor-return.md in 3aa5fbf Nov 6, 2019

I think this example would be clearer if the constructor had statements in the body. Something like:

constructor(a) {
    this.a = a;
}
@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented on docs/rules/no-constructor-return.md in 3aa5fbf Nov 6, 2019

I think this example would be clearer if the constructor had statements in the body. Something like:

constructor(a) {
    this.a = a;
    return 'a';
}

This comment has been minimized.

Copy link
Member Author

g-plane replied Nov 6, 2019

Updated at later commit.

docs/rules/no-constructor-return.md Outdated Show resolved Hide resolved
lib/rules/no-constructor-return.js Outdated Show resolved Hide resolved
tests/lib/rules/no-constructor-return.js Outdated Show resolved Hide resolved
tools/rule-types.json Outdated Show resolved Hide resolved
@g-plane
Copy link
Member Author

g-plane commented Nov 13, 2019

Can anyone tell me why CI would fail?

@mdjermanovic
Copy link
Member

mdjermanovic commented Nov 13, 2019

Looks like an issue with the new version of import-fresh which was released 2 hours ago.

@mdjermanovic
Copy link
Member

mdjermanovic commented Nov 13, 2019

Build seems to work well now with the import-fresh patch release.

Copy link
Member

platinumazure left a comment

Left one documentation suggestion, and one suggestion about the implementation approach. Let me know if I've misunderstood anything. Thanks for putting this together!

docs/rules/no-constructor-return.md Outdated Show resolved Hide resolved
lib/rules/no-constructor-return.js Outdated Show resolved Hide resolved
g-plane added 3 commits Nov 19, 2019
@g-plane g-plane requested review from platinumazure and aladdin-add Nov 19, 2019
Copy link
Member

platinumazure left a comment

Looks good, thanks! Personally, I think this implementation is much more maintainable.

Copy link
Member

kaicataldo left a comment

One small typo, but otherwise LGTM!

docs/rules/no-constructor-return.md Outdated Show resolved Hide resolved
Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@kaicataldo
Copy link
Member

kaicataldo commented Nov 20, 2019

@aladdin-add Have your concerns been addressed?

Copy link
Member

aladdin-add left a comment

LGTM, thanks!

g-plane added 2 commits Nov 20, 2019
@g-plane
Copy link
Member Author

g-plane commented Nov 20, 2019

I've added more cases to prevent crashing.

@aladdin-add aladdin-add merged commit 1a2eb99 into master Nov 21, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191120.9 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@aladdin-add aladdin-add deleted the issue-12481 branch Nov 21, 2019
@eslint eslint bot locked and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.