chore: update eslint + prettier rules #254

ftonato · 2019-10-06T13:36:24Z

Summary:

Add a more opiniated linter: Prettier

Closes #253

closes #253

CLAassistant · 2019-10-06T13:36:30Z

All committers have signed the CLA.

amaury1093 · 2019-10-06T17:47:33Z

Thanks! I hope it was clear in the issue, but the scope of #253 also contains fixing the ~500 linting issues we have now.

ftonato · 2019-10-06T18:52:42Z

Hey there,
I saw it, but I prefer to separate things and I'll send another Pull-request with these fix, hold for that... ⏳

amaury1093 · 2019-10-06T19:13:13Z

Ah, I see! I would still prefer 1 PR for all this, because I will not merge a PR with a red CI.

ftonato · 2019-10-06T21:21:39Z

Ahh okay okay, just wait for it (please). I'll fix here =)

ftonato · 2019-10-07T09:09:55Z

Hello @amaurymartiny,

I have a few doubts about this linting issues, let me know what do you think about them:

Some of the most common errors:

(33 occurrences) Forbidden non-null assertion @typescript-eslint/no-non-null-assertion
(02 occurrences) Type assertion on object literals is forbidden, use a type annotation instead @typescript-eslint/no-object-literal-type-assertion

Some of the most common warnings:

(173 occurrences) Missing return type on function @typescript-eslint/explicit-function-return-type

I think we can ignore this for a while, disabling the rule for all files, right?

(33 occurrences) Unexpected any. Specify a different type @typescript-eslint/no-explicit-any

In the file App/Screens/Search/fetchAlgolia.ts#47, we have the following problem:
What is the impact of changing this to a camel case?

amaury1093 · 2019-10-07T09:48:08Z

(33 occurrences) Forbidden non-null assertion @typescript-eslint/no-non-null-assertion

This one is important! It might catch some bugs, so I wish to keep it.

Basically, for example https://github.com/amaurymartiny/shoot-i-smoke/blob/9641cf6437f9043fd95295a67f8ebcb77687fdc0/App/Screens/Home/Home.tsx#L44

you would need to write:

if (!api) {
  throw new Error('Home.tsx only reachable when `api` is defined.'); // Or some message like this
}

const cigarettesPerDay = api.shootISmoke.cigarettes;

(02 occurrences) Type assertion on object literals is forbidden, use a type annotation instead @typescript-eslint/no-object-literal-type-assertion

I would like to keep these two too. I didn't find where they were, if you need help for those two I can have a look too.

Warnings

This PR seems bigger than I thought. In this case you can do

  rules: {
    '@typescript-eslint/no-explicit-any': 0,
    '@typescript-eslint/explicit-function-return-type': 0
  },

for now in .eslintrc.js, we can do those in a separate PR.

In the file App/Screens/Search/fetchAlgolia.ts#47, we have the following problem:
What is the impact of changing this to a camel case?

Algolia gives snake_case here, so here the only solution would be a eslint-disable-next-line

App/Screens/About/About.tsx

add rule to avoid warnings with: - @typescript-eslint/explicit-function-return-type - @typescript-eslint/no-explicit-any - react/prop-types

ftonato · 2019-10-08T12:08:50Z

I found these errors:

App/Screens/Screens.tsx
44:10 error Type assertion on object literals is forbidden, use a type annotation instead @typescript-eslint/no-object-literal-type-assertion
App/stores/fetchGpsPosition/fetchGpsPosition.ts
48:10 error Type assertion on object literals is forbidden, use a type annotation instead @typescript-eslint/no-object-literal-type-assertion

Do you have any suggestion or would you like to fix them?

amaury1093

This is very, very, very cool. Thanks a lot 🎉

Some little nits:

replace all && || with ?:
replace if (currentLocation === undefined || !Object.keys(currentLocation).length) with simply if(!currentLocation) is enough

App/stores/fetchGpsPosition/fetchGpsPosition.ts

You can remove the as Location cast

App/Screens/Screens.tsx

You can replace the function with

function stackNavigatorOptions(
  initialRouteName: string
): CreateNavigatorConfig<
  NavigationStackConfig,
  NavigationStackRouterConfig,
  NavigationStackOptions,
  NavigationStackProp<NavigationRoute, any>
> {
  return {
    cardStyle: {
      backgroundColor: theme.backgroundColor
    },
    headerMode: 'none',
    initialRouteName,
    defaultNavigationOptions: {
      // FIXME the `headerVisible` field has been moved away from this config
      // @ts-ignore
      headerVisible: false
    }
  };
}

And after that I think we're ready to merge!

.eslintrc.js

App/Screens/Details/Details.tsx

App/Screens/Home/Footer/Footer.tsx

package.json

Co-Authored-By: Amaury Martiny <[email protected]>

amaury1093

Looks good! Just tested the code, nothing broke, so that's perfect.

I'll merge once the CLA is signed and conflicts solved.

ftonato · 2019-10-08T16:26:55Z

Time to merge my friend 🦄

amaury1093 · 2019-10-08T18:53:08Z

Thanks, almost there but not yet! There are 2 errors on Travis, surely because of the master merge. The DistanceUnit type comes from stores/distanceUnit.tsx now

ftonato · 2019-10-08T21:55:24Z

My bad, I did merge using GitHub instead of the command-line interface, but I hope that it's correct now 🚀 🔬

amaury1093

There are some regression errors due to merge.

App/Screens/About/About.tsx

App/Screens/Home/Header/Header.tsx

App/Screens/Details/Distance/Distance.tsx

App/Screens/Home/Header/Header.tsx

Co-Authored-By: Amaury Martiny <[email protected]>

App/Screens/About/About.tsx

App/Screens/ErrorScreen/ErrorScreen.tsx

App/Screens/ShareScreen/ShareScreen.tsx

amaury1093

Sorry, 2 more merge errors. Should be the last 2 ones!

App/Screens/Home/Footer/Footer.tsx

App/Screens/Details/Details.tsx

ftonato · 2019-10-10T08:44:27Z

I will never merge again using GitHub 🐛

amaury1093

This time everything looks good! A huge thanks to you @ftonato

chore: update eslint + prettier rules

bf8afe8

closes #253

style: fixing some issues based on eslint' rules

6b84b1c

amaury1093 reviewed Oct 7, 2019

View reviewed changes

App/Screens/About/About.tsx Show resolved Hide resolved

ftonato added 2 commits October 8, 2019 12:53

chore: update eslint rules

d7007e8

add rule to avoid warnings with: - @typescript-eslint/explicit-function-return-type - @typescript-eslint/no-explicit-any - react/prop-types

style: fix all forbidden non-null assertion

69a39bd

amaury1093 reviewed Oct 8, 2019

View reviewed changes

ftonato and others added 3 commits October 8, 2019 15:23

Update App/Screens/Home/Footer/Footer.tsx

2d49714

Co-Authored-By: Amaury Martiny <[email protected]>

Update App/Screens/Details/Details.tsx

8c37567

Co-Authored-By: Amaury Martiny <[email protected]>

refactor: remove unnecessary rule of eslint and refactor some methods

ad1a663

amaury1093 approved these changes Oct 8, 2019

View reviewed changes

Merge branch 'master' into master

f384b46

ftonato added 3 commits October 8, 2019 22:42

style: fixing imports

408a526

style: fixing code format (prettier/eslint rules)

751871e

docs: adding me as a contributor

316a4bf

amaury1093 requested changes Oct 8, 2019

View reviewed changes

App/Screens/About/About.tsx Outdated Show resolved Hide resolved

App/Screens/Home/Header/Header.tsx Outdated Show resolved Hide resolved

App/Screens/Details/Distance/Distance.tsx Outdated Show resolved Hide resolved

App/Screens/Home/Header/Header.tsx Outdated Show resolved Hide resolved

ftonato and others added 4 commits October 9, 2019 11:51

Update App/Screens/About/About.tsx

7b60e40

Co-Authored-By: Amaury Martiny <[email protected]>

Update App/Screens/Home/Header/Header.tsx

936fb81

Co-Authored-By: Amaury Martiny <[email protected]>

style: fixing code format (prettier/eslint rules) and merge conflicts

4ecdf34

Merge branch 'master' into master

68187bb

amaury1093 requested changes Oct 9, 2019

View reviewed changes

App/Screens/About/About.tsx Outdated Show resolved Hide resolved

amaury1093 requested changes Oct 9, 2019

View reviewed changes

App/Screens/ErrorScreen/ErrorScreen.tsx Outdated Show resolved Hide resolved

App/Screens/ShareScreen/ShareScreen.tsx Outdated Show resolved Hide resolved

ftonato added 2 commits October 9, 2019 18:39

fix: fixing broken merge

bc31111

fix: fixing broken merge 🐛🐛

ac849e6

amaury1093 requested changes Oct 9, 2019

View reviewed changes

App/Screens/Home/Footer/Footer.tsx Outdated Show resolved Hide resolved

App/Screens/Details/Details.tsx Show resolved Hide resolved

fix: fixing broken merge 🐛🐛🐛

25df90c

amaury1093 approved these changes Oct 10, 2019

View reviewed changes

amaury1093 merged commit 1b0c1ce into shootismoke:master Oct 10, 2019

amaury1093 mentioned this pull request Oct 10, 2019

Make linter work with explicit-function-return-type and no-any #274

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update eslint + prettier rules #254

chore: update eslint + prettier rules #254

ftonato commented Oct 6, 2019

CLAassistant commented Oct 6, 2019 •

edited

Loading

amaury1093 commented Oct 6, 2019

ftonato commented Oct 6, 2019

amaury1093 commented Oct 6, 2019

ftonato commented Oct 6, 2019

ftonato commented Oct 7, 2019

amaury1093 commented Oct 7, 2019 •

edited

Loading

ftonato commented Oct 8, 2019

amaury1093 left a comment

amaury1093 left a comment

ftonato commented Oct 8, 2019

amaury1093 commented Oct 8, 2019

ftonato commented Oct 8, 2019

amaury1093 left a comment

amaury1093 left a comment

ftonato commented Oct 10, 2019

amaury1093 left a comment

chore: update eslint + prettier rules #254

chore: update eslint + prettier rules #254

Conversation

ftonato commented Oct 6, 2019

Summary:

CLAassistant commented Oct 6, 2019 • edited Loading

amaury1093 commented Oct 6, 2019

ftonato commented Oct 6, 2019

amaury1093 commented Oct 6, 2019

ftonato commented Oct 6, 2019

ftonato commented Oct 7, 2019

amaury1093 commented Oct 7, 2019 • edited Loading

ftonato commented Oct 8, 2019

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

ftonato commented Oct 8, 2019

amaury1093 commented Oct 8, 2019

ftonato commented Oct 8, 2019

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

ftonato commented Oct 10, 2019

amaury1093 left a comment

Choose a reason for hiding this comment

CLAassistant commented Oct 6, 2019 •

edited

Loading

amaury1093 commented Oct 7, 2019 •

edited

Loading