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

Conversation

g-plane
Copy link
Member

@g-plane 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?

@g-plane g-plane added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint labels Nov 5, 2019
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

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

@mdjermanovic
Copy link
Member

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

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

One small typo, but otherwise LGTM!

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

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kaicataldo
Copy link
Member

@aladdin-add Have your concerns been addressed?

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@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
@aladdin-add aladdin-add deleted the issue-12481 branch November 21, 2019 02:19
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 21, 2020
@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 May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants