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

Conversation

@gilbsgilbs
Copy link
Contributor

@gilbsgilbs gilbsgilbs commented Feb 12, 2019

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 codecov bot commented Feb 12, 2019

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%> (ø) ⬆️
Copy link
Member

@bradzacher bradzacher left a comment

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;
@gilbsgilbs gilbsgilbs force-pushed the gilbsgilbs:explicit-variable-type-arrow-function branch from 3ec706f to a9df636 Feb 12, 2019
@gilbsgilbs
Copy link
Contributor Author

@gilbsgilbs gilbsgilbs commented Feb 12, 2019

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 gilbsgilbs:explicit-variable-type-arrow-function branch 3 times, most recently from 871c9b1 to d08e435 Feb 12, 2019
@gilbsgilbs gilbsgilbs force-pushed the gilbsgilbs:explicit-variable-type-arrow-function branch from d08e435 to 8a73cee Feb 14, 2019
@gilbsgilbs gilbsgilbs force-pushed the gilbsgilbs:explicit-variable-type-arrow-function branch 2 times, most recently from db9f929 to 17acfba Feb 15, 2019
@gilbsgilbs gilbsgilbs force-pushed the gilbsgilbs:explicit-variable-type-arrow-function branch from 17acfba to 072afa5 Feb 16, 2019
@gilbsgilbs
Copy link
Contributor Author

@gilbsgilbs gilbsgilbs commented Feb 18, 2019

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

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Feb 19, 2019

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.

@gilbsgilbs
Copy link
Contributor Author

@gilbsgilbs gilbsgilbs commented Feb 19, 2019

@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.

@nimaa77
Copy link

@nimaa77 nimaa77 commented Mar 14, 2019

any updates on this ?

@gilbsgilbs
Copy link
Contributor Author

@gilbsgilbs gilbsgilbs commented Mar 20, 2019

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
8 checks passed
8 checks passed
@semantic-pull-requests
Semantic Pull Request ready to be squashed
Details
@codecov
codecov/patch 100% of diff hit (target 90%)
Details
@codecov
codecov/project 97.2% (+<.01%) compared to c7db594
Details
@azure-pipelines
typescript-eslint.typescript-eslint Build #20190321.1 succeeded
Details
@azure-pipelines
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
@azure-pipelines
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
@azure-pipelines
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_6_x) Run unit tests on other Node.js versions node_6_x succeeded
Details
@azure-pipelines
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details
@gilbsgilbs gilbsgilbs deleted the gilbsgilbs:explicit-variable-type-arrow-function branch Mar 21, 2019
@gilbsgilbs
Copy link
Contributor Author

@gilbsgilbs gilbsgilbs commented Mar 21, 2019

Thanks for the reviews and your time.

@garycourt
Copy link

@garycourt garycourt commented Mar 22, 2019

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

@saranshkataria
Copy link

@saranshkataria saranshkataria commented Mar 23, 2019

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
kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this pull request Aug 27, 2019
…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
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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants