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

Updated lint config #89

Merged
merged 5 commits into from
Jun 26, 2019
Merged

Updated lint config #89

merged 5 commits into from
Jun 26, 2019

Conversation

adamgajzlerowicz
Copy link
Contributor

This PR:

  • extends linting configuration with extra plugins
  • adds prettier for unified experience across projects
  • add husky for pre push lint check, to ensure only healthy code in the main repo

Copy link
Contributor

@Bosmanfrx Bosmanfrx left a comment

Choose a reason for hiding this comment

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

Generally fine. But IMO it should be splitted in 2 PRs. One adding Prettier and second one with just a reformat, when all PRs are closed (avoids unnecessary conflicts). Just one comment about adding Prettier.

"node": true
},
"plugins": [
"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, that prettier plugin to eslint is unnecessary. Prettier itself should be transparent to development process. It should be added on pre-commit hook instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current strategy uses prettier as a plugin, thus there is no need to use prettier directly as eslint uses it for linting. Prehook is also already added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, you need to run prettier yourself or manually adjust code. Why not just hook prettier to pre-commit to do it for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is already done.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not. In pre-commit hook yarn lint is ran and not yarn lint:fix. Also changed files need to be restaged in commit if you decide to use yarn lint:fix. Also IMHO adding Prettier as plugin adds unnecessary information noise in IDE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only check with no fix is used so you can stage only part of the changes. Making fix and staging files for devs will cause unwanted changes to go to repository

Copy link
Contributor

@OtisRed OtisRed left a comment

Choose a reason for hiding this comment

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

@Bosmanfrx I will force merge for now but if you want to continue the discussion please create a new issue :)

@OtisRed OtisRed merged commit bc9241d into CodeForPoznan:master Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants