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): allow explicit variable type with arrow functions #260

Merged
merged 11 commits into from
Mar 21, 2019
Merged

feat(eslint-plugin): allow explicit variable type with arrow functions #260

merged 11 commits into from
Mar 21, 2019

Conversation

gilbsgilbs
Copy link
Contributor

Fixes #149

Not sure if this is really acceptable though. Please tell me if it has some edge cases I didn't see or if it's just too broad.

@codecov
Copy link

codecov bot commented Feb 12, 2019 •
edited
Loading

Codecov Report

Merging #260 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #260      +/-   ##
=========================================
+ Coverage    97.2%   97.2%   +<.01%     
=========================================
  Files          68      68              
  Lines        2501    2505       +4     
  Branches      385     387       +2     
=========================================
+ Hits         2431    2435       +4     
  Misses         44      44              
  Partials       26      26
Impacted Files Coverage Δ
...-plugin/src/rules/explicit-function-return-type.ts 100% <100%> (ø) ⬆️

bradzacher
bradzacher requested changes Feb 12, 2019
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Please add an example to the rule's readme so people can see this functionality without looking at the tests.

@typescript-eslint/core-team do we want to put this behind a setting (default on) in case people don't want to "trust" type annotations?
I.e. might not trust them because the following case passes:

type X = Function;
const x: X = () => 1;

packages/eslint-plugin/src/rules/explicit-function-return-type.ts Outdated Show resolved Hide resolved
@gilbsgilbs gilbsgilbs force-pushed the explicit-variable-type-arrow-function branch from 3ec706f to a9df636 Compare February 12, 2019 21:17
@gilbsgilbs
Copy link
Contributor Author

Thanks for the review @bradzacher .

Please add an example to the rule's readme so people can see this functionality without looking at the tests.

Done.

@typescript-eslint/core-team do we want to put this behind a setting (default on) in case people don't want to "trust" type annotations?
I.e. might not trust them because the following case passes:

type X = Function;
const x: X = () => 1;

I think you are right. I made this opt-in.

@gilbsgilbs gilbsgilbs force-pushed the explicit-variable-type-arrow-function branch 3 times, most recently from 871c9b1 to d08e435 Compare February 14, 2019 21:15
@gilbsgilbs gilbsgilbs force-pushed the explicit-variable-type-arrow-function branch from d08e435 to 8a73cee Compare February 14, 2019 21:39
@bradzacher bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label Feb 14, 2019
@gilbsgilbs gilbsgilbs force-pushed the explicit-variable-type-arrow-function branch 2 times, most recently from db9f929 to 17acfba Compare February 16, 2019 12:55
@gilbsgilbs gilbsgilbs force-pushed the explicit-variable-type-arrow-function branch from 17acfba to 072afa5 Compare February 16, 2019 13:00
@gilbsgilbs
Copy link
Contributor Author

@bradzacher @j-f1 , is there still something preventing this PR from being merged?

@bradzacher
Copy link
Member

Please avoid the workflow of amend + force push.
When you use that workflow, it means your branch has only one commit in it.
This means that if we review your code + suggest changes, then you make the changes and commit them via amending we (and git, and github) can't tell the branch state before + after the review.
The result of that is that we have to review the entire PR if you force push so that we can make sure that the changes are what we expect.

bradzacher
bradzacher previously approved these changes Feb 19, 2019
View reviewed changes
@gilbsgilbs
Copy link
Contributor Author

@bradzacher I didn't realize GitHub couldn't handle this properly. Also, I'm not very used to commitlint and was unsure what commit messages I should use for non-features commits. I'll do my best to avoid force-pushes in the future. All my apologies for the inconvenience and thanks for your time and review.

@gilbsgilbs gilbsgilbs mentioned this pull request Feb 19, 2019
Closed
@nimaa77
Copy link

nimaa77 commented Mar 14, 2019

any updates on this ?

@gilbsgilbs
Copy link
Contributor Author

Hi! Can we unblock this @bradzacher @JamesHenry? Are we waiting for something? Thanks!

@bradzacher bradzacher merged commit bea6b92 into typescript-eslint:master Mar 21, 2019
@gilbsgilbs gilbsgilbs deleted the explicit-variable-type-arrow-function branch March 21, 2019 07:35
@gilbsgilbs
Copy link
Contributor Author

Thanks for the reviews and your time.

@garycourt
Copy link

Has this been released to NPM yet? If not, when could one expect it?

@saranshkataria
Copy link

I have been waiting for the release since the merge too! It hasn't been released yet from what I can see.

trivikr added a commit to trivikr/redux-react-hook-todo-ts that referenced this pull request Mar 25, 2019
@trivikr
Disable "@typescript-eslint/explicit-function-return-type"
3878cad 
Refs: typescript-eslint/typescript-eslint#260
ToDo: Enable when fix is available
kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this pull request Aug 27, 2019
@bradzacher @JamesHenry
[FEAT] [BREAKING] [no-unused-vars] remove implicit dep on base rule (t…
8229d06 
…ypescript-eslint#260)

Similar to what we've done with both indent, and camelcase.
Rather than forcing users to configure both our rule and the base rule (which has been a source of confusion), this lets users just use our rule.

```diff
 {
-    "no-unused-vars": ["error", { config }],
+    "no-unused-vars": "off",
-    "typescript/no-unused-vars": "error",
+    "typescript/no-unused-vars": ["error", { config }],
 }
```
kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this pull request Aug 27, 2019
@bradzacher @JamesHenry
[FEAT] Recommended config (typescript-eslint#261)
32753d1 
Fixes typescript-eslint#144
Requires ~~typescript-eslint#259~~, ~~typescript-eslint#260~~.

- added a util to make it standardised and easier to add default config for a rule
- configured recommended based on typescript-eslint#144
  - purposely switched `recommended` prop to be `"error" | "warning" | false`
    - inside the eslint repo, it should be `true`. otherwise it's just a property that isn't used officially by eslint. It's truthy so `eslint-docs` still work.
  - changed recommended generator to accept `"error"`/`"warning"` for more configurability.
- adjusted default config of certain rules that didn't match our recommendations.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@j-f1 j-f1 j-f1 left review comments

@bradzacher bradzacher bradzacher approved these changes

@JamesHenry JamesHenry JamesHenry left review comments

Assignees
No one assigned
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[explicit-function-return-type] Don't report when using an explicit variable type with an arrow function
7 participants
@gilbsgilbs @bradzacher @nimaa77 @garycourt @saranshkataria @JamesHenry @j-f1