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

feat(eslint-plugin): [explicit-function-return-type] add allowedNames #4440

Merged
merged 15 commits into from Feb 2, 2022

Conversation

islandryu
Copy link
Contributor

@islandryu islandryu commented Jan 14, 2022

Add option allowedNames to explicit-function-return-type

PR Checklist

Overview

You may pass function/method names you would like this rule to ignore, like so:

{
  "@typescript-eslint/explicit-function-return-type": [
    "error",
    {
      "allowedNames": ["ignoredFunctionName", "ignoredMethodName"]
    }
  ]
}

@nx-cloud
Copy link

nx-cloud bot commented Jan 14, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 65103ff. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


Successfully ran 48 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

typescript-eslint bot commented Jan 14, 2022

Thanks for the PR, @islandryu!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@netlify
Copy link

netlify bot commented Jan 14, 2022

Deploy Preview for typescript-eslint failed.

🔨 Explore the source changes: 65103ff

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/61f8ee03e1dd9e00086c7f6a

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #4440 (65103ff) into main (04baac8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4440      +/-   ##
==========================================
+ Coverage   94.54%   94.56%   +0.01%     
==========================================
  Files         147      147              
  Lines        7851     7877      +26     
  Branches     2516     2530      +14     
==========================================
+ Hits         7423     7449      +26     
  Misses        234      234              
  Partials      194      194              
Flag Coverage Δ
unittest 94.56% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...-plugin/src/rules/explicit-function-return-type.ts 100.00% <100.00%> (ø)

@islandryu islandryu marked this pull request as ready for review Jan 14, 2022
@bradzacher bradzacher added the enhancement: plugin rule option label Jan 14, 2022
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

LGTM in general, thanks!

Codecov is right that there's a gap in test coverage; could you please
add a bit more testing and resolve the conflicts from the main branch? Once you do I think we should be good to merge.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response label Jan 23, 2022
@islandryu islandryu requested a review from JoshuaKGoldberg Jan 29, 2022
@islandryu
Copy link
Contributor Author

islandryu commented Jan 29, 2022

@JoshuaKGoldberg
Thank you for review!
I resolved conflict and improved coverage.

I apologize for the large number of commits.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jan 29, 2022

Large number of commits is no problem 😄, I do that all the time. We squash PR merges anyway and most PRs -including this one- are a small enough number of changes that we don't particularly need a Git walkthrough. Thanks!

JoshuaKGoldberg
JoshuaKGoldberg previously approved these changes Jan 29, 2022
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Super, thanks so much @islandryu! 🚀

@JoshuaKGoldberg JoshuaKGoldberg enabled auto-merge (squash) Jan 29, 2022
@islandryu
Copy link
Contributor Author

islandryu commented Jan 31, 2022

@JoshuaKGoldberg
Sorry, I updated the PR by mistake.
Is there anything left for me to do regarding this PR?

@islandryu islandryu requested a review from JoshuaKGoldberg Jan 31, 2022
@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jan 31, 2022

Nope, it's ready to merge!

I don't have the ability to merge on a failing build and the Netlify deployment is nondeterministically failing. cc @bradzacher

@bradzacher
Copy link
Member

bradzacher commented Jan 31, 2022

@JoshuaKGoldberg - I've removed netlify from the "required" list of checks, so you should be able to merge past that signal from now on!

@bradzacher bradzacher disabled auto-merge Jan 31, 2022
Copy link
Member

@bradzacher bradzacher left a comment

just one small comment about correctness - otherwise great work!

case AST_NODE_TYPES.MethodDefinition:
case AST_NODE_TYPES.PropertyDefinition:
case AST_NODE_TYPES.Property: {
if (parent.key.type === AST_NODE_TYPES.Identifier) {
Copy link
Member

@bradzacher bradzacher Jan 31, 2022

Choose a reason for hiding this comment

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

one minor problem with this code:

const ignoredName = 'notIgnoredName';
class Foo {
  [ignoredName]() {}
}

This is valid code! Which obviously should not be ignored.
Only checking for .key.type === 'Identifier' will match this case.
You should also assert that .computed === false.

cc @JoshuaKGoldberg as an FYI - this is a really common case that everyone (including me!) always forgets for properties of all kinds AND for member expressions as well.
I raised the issue with the estree spec but the outcome there was really "spec can't change".

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Jan 31, 2022

Choose a reason for hiding this comment

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

Ah thanks for the spot!

Copy link
Contributor Author

@islandryu islandryu Feb 1, 2022

Choose a reason for hiding this comment

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

Thank you!
I didn't know such a pattern existed.
@bradzacher
I changed the computed name so that it is not ignored by the allowedName.

Copy link
Member

@bradzacher bradzacher left a comment

nice one! LGTM

@bradzacher bradzacher merged commit 936e252 into typescript-eslint:main Feb 2, 2022
12 of 17 checks passed
@islandryu islandryu deleted the featAddallowedNames branch Feb 9, 2022
lonyele pushed a commit to lonyele/typescript-eslint that referenced this issue Feb 12, 2022
…typescript-eslint#4440)

* feat(eslint-plugin): [explicit-function-return-type] add allowedNames option

* feat(eslint-plugin): [explicit-function-return-type] afix typecheck

* Update packages/eslint-plugin/src/rules/explicit-function-return-type.ts

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>

* feat(eslint-plugin): [explicit-function-return-type] Change to allowedNames to work for object properties and class methods

* feat(eslint-plugin): [explicit-function-return-type] Change to allowedNames to work for object properties and class methods

* fix(eslint-plugin): [explicit-function-return-type] fix for codecov

* fix(eslint-plugin): [explicit-function-return-type] add test

* fix(eslint-plugin): [explicit-function-return-type] add test

* fix(eslint-plugin): [explicit-function-return-type] fix for codecov

* fix(eslint-plugin): [explicit-function-return-type] Change allowedName to not ignore computed property

Co-authored-by: Josh Goldberg <me@joshuakgoldberg.com>
Co-authored-by: Josh Goldberg <joshuakgoldberg@outlook.com>
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Feb 16, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.10.2` -> `5.12.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.10.2/5.12.0) |
| [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.10.2` -> `5.12.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.10.2/5.12.0) |

---

### Release Notes

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/eslint-plugin)</summary>

### [`v5.12.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;5120-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5110v5120-2022-02-14)

[Compare Source](typescript-eslint/typescript-eslint@v5.11.0...v5.12.0)

##### Bug Fixes

-   **eslint-plugin:** \[init-declarations] fix nested namespace ([#&#8203;4544](typescript-eslint/typescript-eslint#4544)) ([fe910e6](typescript-eslint/typescript-eslint@fe910e6))
-   **eslint-plugin:** \[no-unnecessary-type-arguments] Use Symbol to check if it's the same type ([#&#8203;4543](typescript-eslint/typescript-eslint#4543)) ([5b7d8df](typescript-eslint/typescript-eslint@5b7d8df))
-   support nested object deconstructuring with type annotation ([#&#8203;4548](typescript-eslint/typescript-eslint#4548)) ([4da9278](typescript-eslint/typescript-eslint@4da9278))

##### Features

-   add checking property definition for allowNames option ([#&#8203;4542](typescript-eslint/typescript-eslint#4542)) ([e32bef6](typescript-eslint/typescript-eslint@e32bef6))

### [`v5.11.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;5110-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5102v5110-2022-02-07)

[Compare Source](typescript-eslint/typescript-eslint@v5.10.2...v5.11.0)

##### Bug Fixes

-   **eslint-plugin:** \[no-magic-numbers] fix invalid schema merging ([#&#8203;4517](typescript-eslint/typescript-eslint#4517)) ([b95f796](typescript-eslint/typescript-eslint@b95f796))
-   **eslint-plugin:** \[non-nullable-type-assertion-style] fix false positive when asserting to a generic type that might be nullish ([#&#8203;4509](typescript-eslint/typescript-eslint#4509)) ([4209362](typescript-eslint/typescript-eslint@4209362))

##### Features

-   **eslint-plugin:** \[explicit-function-return-type] add allowedNames ([#&#8203;4440](typescript-eslint/typescript-eslint#4440)) ([936e252](typescript-eslint/typescript-eslint@936e252))

#### [5.10.2](typescript-eslint/typescript-eslint@v5.10.1...v5.10.2) (2022-01-31)

##### Bug Fixes

-   **eslint-plugin:** \[no-restricted-imports] allow relative type imports with patterns configured ([#&#8203;4494](typescript-eslint/typescript-eslint#4494)) ([4a6d217](typescript-eslint/typescript-eslint@4a6d217))

#### [5.10.1](typescript-eslint/typescript-eslint@v5.10.0...v5.10.1) (2022-01-24)

**Note:** Version bump only for package [@&#8203;typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin)

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/parser)</summary>

### [`v5.12.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;5120-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5110v5120-2022-02-14)

[Compare Source](typescript-eslint/typescript-eslint@v5.11.0...v5.12.0)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

### [`v5.11.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;5110-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5102v5110-2022-02-07)

[Compare Source](typescript-eslint/typescript-eslint@v5.10.2...v5.11.0)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

#### [5.10.2](typescript-eslint/typescript-eslint@v5.10.1...v5.10.2) (2022-01-31)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

#### [5.10.1](typescript-eslint/typescript-eslint@v5.10.0...v5.10.1) (2022-01-24)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone@noreply.codeberg.org>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1157
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response enhancement: plugin rule option
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants