Skip to content
This repository was archived by the owner on Nov 18, 2024. It is now read-only.

Conversation

willdurand
Copy link
Member

@willdurand willdurand commented Jan 29, 2019

Fixes #17
Fixes #75


There are 3 warnings due to JSX not being supported by ESLint anymore: https://eslint.org/blog/2015/03/eslint-0.17.0-released#changes-to-jsxreact-handling.

I think it could maybe be fixed automagically by upgrading some of our dependencies, but it requires some work on https://github.com/mozilla/eslint-config-amo (which is on my todo-list).

@willdurand willdurand force-pushed the eslint branch 2 times, most recently from 83b3637 to ffa92f3 Compare January 29, 2019 14:31
@codecov-io
Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #74 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #74   +/-   ##
=======================================
  Coverage   62.59%   62.59%           
=======================================
  Files          10       10           
  Lines         131      131           
  Branches       26       26           
=======================================
  Hits           82       82           
  Misses         45       45           
  Partials        4        4
Impacted Files Coverage Δ
src/configureStore.tsx 62.5% <ø> (ø) ⬆️
src/reducers/example.tsx 85.71% <ø> (-1.79%) ⬇️
src/index.tsx 0% <ø> (ø) ⬆️
src/components/App/index.tsx 38.46% <ø> (ø) ⬆️
src/server/index.js 63.88% <ø> (ø) ⬆️
src/api/index.tsx 100% <100%> (ø) ⬆️
src/components/DiffView/index.tsx 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a900ade...b425d0b. Read the comment docs.

@bobsilverberg bobsilverberg self-assigned this Jan 29, 2019
bobsilverberg
bobsilverberg previously approved these changes Jan 29, 2019
Copy link
Contributor

@bobsilverberg bobsilverberg left a comment

Choose a reason for hiding this comment

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

This looks good to me, and it works well with my IDE as well. Nice job @willdurand!

@kumar303
Copy link
Contributor

If it's not too crazy, how about squeezing this in? https://github.com/prettier/eslint-config-prettier Otherwise it could be done in a separate PR. Disabling the rules made moot by prettier would be nice and could possibly speed up our linting.

@willdurand
Copy link
Member Author

I pushed a new commit with more types.. The ESLint TS config wanted moaar!

@willdurand willdurand requested review from bobsilverberg and removed request for bobsilverberg January 29, 2019 18:27
@willdurand willdurand dismissed bobsilverberg’s stale review January 29, 2019 18:27

I (will) made some changes

@willdurand
Copy link
Member Author

So upgrading the eslint dependencies did not remove the warnings:

$ eslint --ext .js --ext tsx src/

/Users/williamdurand/projects/mozilla/addons-code-manager/src/components/App/index.test.tsx
  19:5  warning  'authToken' is assigned a value but never used  @typescript-eslint/no-unused-vars

/Users/williamdurand/projects/mozilla/addons-code-manager/src/components/App/index.tsx
  5:8  warning  'styles' is defined but never used  @typescript-eslint/no-unused-vars

/Users/williamdurand/projects/mozilla/addons-code-manager/src/components/LoginButton/index.tsx
  3:8  warning  'styles' is defined but never used  @typescript-eslint/no-unused-vars

/Users/williamdurand/projects/mozilla/addons-code-manager/src/index.tsx
  11:7  warning  'store' is assigned a value but never used  @typescript-eslint/no-unused-vars

✖ 4 problems (0 errors, 4 warnings)

@willdurand
Copy link
Member Author

rebased again

@willdurand
Copy link
Member Author

I found the issue related to the JSX and false-positives, a fix has been merged < 24h ago: typescript-eslint/typescript-eslint#161 and I confirm it works.

@willdurand
Copy link
Member Author

This is now ready 🚀

@willdurand
Copy link
Member Author

I found the issue related to the JSX and false-positives, a fix has been merged < 24h ago: typescript-eslint/typescript-eslint#161 and I confirm it works.

I added another commit to use the canary release of the plugin.

@kumar303
Copy link
Contributor

If it's not too crazy, how about squeezing this in? https://github.com/prettier/eslint-config-prettier

I deferred it to #84

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Here is an incomplete review before the status meeting.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

...continuing my review

@willdurand willdurand force-pushed the eslint branch 2 times, most recently from 257ef90 to b425d0b Compare January 31, 2019 09:39
@willdurand willdurand requested a review from kumar303 January 31, 2019 09:40
@willdurand
Copy link
Member Author

I fixed all the comments, thanks!

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

r+wc

Just a few minor things to clean up

@willdurand willdurand merged commit 8e681f5 into master Jan 31, 2019
@willdurand willdurand deleted the eslint branch January 31, 2019 19:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a lint rule to disallow .ts files, enforce .tsx Set up code linting
4 participants