Skip to content
This repository has been archived by the owner on Dec 13, 2020. It is now read-only.

Linting setup #1259

Merged
merged 29 commits into from
Dec 11, 2017
Merged

Linting setup #1259

merged 29 commits into from
Dec 11, 2017

Conversation

pablosichert
Copy link
Contributor

#1206

This PR includes the new configuration for the linting setup and automatic code transformation, without touching any code yet.

I propose we take a week to agree on the settings and transform the code afterwards.

Please review and add your suggestions @wiadev and @ottosichert.

@pablosichert
Copy link
Contributor Author

You can see by the rising error count that Code Climate internally uses ESLint and checks the rules configured by the project.

.eslintrc Outdated
"prettier/prettier": [
"error",
{
"tabWidth": 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer 2 spaces for React applications as some components require deeper nesting in their render() methods and a max-len of 80 forces wrapping JSX statements too often

Also this is the perfect opportunity to ever change something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be fine with that

.eslintrc Outdated
"es6": true
},
"parser": "babel-eslint",
"extends": ["eslint:recommended", "plugin:react/recommended", "prettier"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think eslint-config-airbnb is solid and I usually use it among eslint:recommended and plugin:react/recommended. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@teosarca
Copy link
Member

@pablosichert @ottosichert @wiadev
hi guys,

pls ping me when it's ready to be integrated.

@teosarca
Copy link
Member

@pablosichert pls solve the package.json conficts.

@pablosichert
Copy link
Contributor Author

I reverted the formatting changes in package.json and rebased with master. Will leave any changes to that to the end.

@pablosichert
Copy link
Contributor Author

Vote 👍 or 👎 : Dangling commas

@teosarca
Copy link
Member

+1 for dangling comma "always-multiline".
But I guess, as a transition we shall go with "only-multiline".

@pablosichert
Copy link
Contributor Author

Let's put the vote on the comment directly, more polls will follow.

Every option we decide for will automatically be transformed for the entire codebase. So we can take the option we find most optimal, regardless of how the code looks right now. 😄

@ottosichert
Copy link
Contributor

Hint for next polls: Use multiple reactions for each rule option:

  • 👎 never
  • ❤️ always
  • 👍 always-multiline
  • 😕 only-multiline

  • 😄 unused
  • 🎉 unused
:-1:
:heart:
:+1:
:confused:
:smile:
:tada:

@metas-ts
Copy link
Member

Hi @pablosichert @ottosichert
i recomment to consider if it's ready to be integrated now and to ping teo if it is the case

@pablosichert
Copy link
Contributor Author

pablosichert commented Oct 23, 2017

Hey @metas-ts, config is mostly done, but it'll take me a day to manually resolve edge cases and to verify that all automatic transformations work flawlessly.

Since there are still some (urging) features on the list for this week, I need to defer this PR to at least Friday.

EDIT: to this point, rebasing with master works without conflicts. (Just did this morning.) But I need to block a day when I want to do all the transformations to make sure master didn't move on.

@pablosichert
Copy link
Contributor Author

We are done with integrating the linter and fixing all eslint issues. (Ignoring react/prop-types and only having them as warning for now, since there are too many to fix in one go.)

The configuration for the linter is https://github.com/prettier/prettier out of the box.

npm run lint should be run on Jenkins from now on

@teosarca teosarca merged commit bca3aac into master Dec 11, 2017
@teosarca teosarca deleted the dev-1206 branch December 11, 2017 19:20
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.

4 participants