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

feat(eslint-plugin): add new rule no-empty-function #626

Merged
merged 2 commits into from
Jun 20, 2019
Merged

feat(eslint-plugin): add new rule no-empty-function #626

merged 2 commits into from
Jun 20, 2019

Conversation

scottohara
Copy link
Contributor

Extends the no-empty-function rule from ESlint core to allow for empty constructor bodies when parameter properties are used (a.k.a. concise constructor functions).

Fixes #426

Examples of allowed code

/*eslint @typescript-eslint/no-empty-function: "error"*/

class Person {
  constructor(private firstName: string, private surname: string) {}
}

...which is functionally equivalent to writing:

class Person {
  private firstName: string;
  private surname: string;

  constructor(firstName: string, surname: string) {
    this.firstName = firstName;
    this.surname = surname;
  }
}

Aside from this one specific case, the rule passes through to ESLint core no-empty-function for everything else, so the following still applies:

Examples of valid code

/*eslint @typescript-eslint/no-empty-function: ["error", { allow: ['constructors'] }]*/

class Person {
  constructor(firstName: string, surname: string) {}
}
/*eslint @typescript-eslint/no-empty-function: ["error", { allow: ['methods'] }]*/

class Person {
  otherMethod(firstName: string, surname: string) {}
}

Examples of invalid code

/*eslint @typescript-eslint/no-empty-function: "error"*/

class Person {
  constructor(firstName: string, surname: string) {}
}
/*eslint @typescript-eslint/no-empty-function: "error"*/

class Person {
  otherMethod(firstName: string, surname: string) {}
}

@scottohara scottohara mentioned this pull request Jun 19, 2019
Closed
@bradzacher bradzacher added the enhancement New feature or request label Jun 19, 2019
@JamesHenry
Copy link
Member

Definitely one of those awkward ones that is not super clear-cut, but overall seems like a reasonable change and I think matches user expectations

@codecov
Copy link

codecov bot commented Jun 20, 2019 •
edited
Loading

Codecov Report

Merging #626 into master will decrease coverage by 0.02%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
- Coverage   94.27%   94.25%   -0.03%     
==========================================
  Files         106      107       +1     
  Lines        4369     4388      +19     
  Branches     1202     1208       +6     
==========================================
+ Hits         4119     4136      +17     
- Misses        145      147       +2     
  Partials      105      105
Impacted Files Coverage Δ
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️
...kages/eslint-plugin/src/rules/no-empty-function.ts 88.88% <88.88%> (ø)

@bradzacher
Copy link
Member

Code LGTM! Thanks for doing this!

@bradzacher bradzacher merged commit 747bfcb into typescript-eslint:master Jun 20, 2019
@scottohara scottohara deleted the no-empty-function branch June 20, 2019 05:25
@joaovieira
Copy link

Just merged an older eslint ruleset (used in JS projects) with a plugin:@typescript-eslint/recommended one and started getting no-useless-constructor and no-empty-function errors as they were enabled in the old ruleset. Just seen both have TS rules now, which is great! ❤️

Do you think these could be added to the recommended ruleset?

@bradzacher
Copy link
Member

We only update recommended rulesets on breaking changes.
We will discuss the changes to the set when it comes time for a breaking change.

@onichandame
Copy link

Thx for the fix. But how can I use this PR? Any rules that I can use to override the recommended ruleset?

@bradzacher
Copy link
Member

please read the rule docs:
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-empty-function.md

@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JamesHenry JamesHenry JamesHenry requested changes

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend no-empty-function to handle constructors with parameter properties
5 participants
@scottohara @JamesHenry @bradzacher @joaovieira @onichandame