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

Fixes #1474 - added pre-commit linter #1538

Merged
merged 4 commits into from
Jun 6, 2017
Merged

Fixes #1474 - added pre-commit linter #1538

merged 4 commits into from
Jun 6, 2017

Conversation

magsout
Copy link
Member

@magsout magsout commented Apr 27, 2017

I just added the pre-commit part #1474 (comment)

r? @zoepage

@magsout magsout changed the title Fixes #1474 - added pre-commit Fixes #1474 - added pre-commit linter Apr 28, 2017
@magsout magsout requested a review from zoepage April 28, 2017 19:28
@zoepage
Copy link
Member

zoepage commented May 3, 2017

@magsout Sorry for the late review. Had issues with my setup.

I get two errors. Could you take a look please? :)

/webcompat/webcompat.com/webcompat/static/js/lib/labels.js
  159:59  error  Follow `prettier` formatting (expected '\n' but found ' ')  prettier/prettier
/webcompat/webcompat.com/webcompat/static/js/lib/models/query-params.js
  278:28  error  Follow `prettier` formatting (expected '\n' but found ' ')  prettier/prettier

✖ 2 problems (2 errors, 0 warnings)

Would you also like to add the information about the pre-linter to that PR? Would be great. Otherwise I'm happy to file a follow up issue and add it.
#1474 (comment)

@magsout
Copy link
Member Author

magsout commented May 3, 2017

I get two errors. Could you take a look please? :)

Yeah, I had exactly these errors. I think we should blocked the version of prettier to avoid this kind of errors. If you check the version of prettier in node_modules it is different than 1.1.0..

@magsout
Copy link
Member Author

magsout commented May 3, 2017

Would you also like to add the information about the pre-linter to that PR? Would be great. Otherwise I'm happy to file a follow up issue and add it.
#1474 (comment)

@zoepage oups, I I have forgotten, I'll do.

Thanks for the review.

@zoepage
Copy link
Member

zoepage commented May 8, 2017

@magsout I know, you are super busy right now (FAMILY always comes FIRST 🎉)
Is there anything I could help with? (If not, don't worry.)

@magsout
Copy link
Member Author

magsout commented May 9, 2017

@magsout I know, you are super busy right now (FAMILY always comes FIRST 🎉)
Is there anything I could help with? (If not, don't worry.)

@zoepage oups, sorry, I'm on it. I'll take care of it Friday night...

@magsout
Copy link
Member Author

magsout commented May 14, 2017

@zoepage sorry for the delay..

I have sent a commit for the documentation. Tell me what do you think ?

`npm run lint` check all JavaScript files and display the resulting errors. If you get errors, you have two possibilities. You can correct yourself or you can run `npm run fix` which use Prettier to format the code.

In order to avoid errors during a Pull Request, `npm run lint` will be executed before each commit.

@@something to write by miketaylr@@

This comment was marked as abuse.

package.json Outdated
"prettier": "^1.1.0",
"suitcss-utils-align": "^0.2.0",
"suitcss-utils-display": "^0.4.0"
"prettier": "^1.1.0"

This comment was marked as abuse.

This comment was marked as abuse.


> Note: All code changes should be made to the files in `lib`

#### Linting

For the consistency of our codebase, we use eslint with these rules: https://github.com/webcompat/webcompat.com/blob/master/.eslintrc. We also use Prettier for formatting our code. We have set up two recipes to check and fixe the codebase.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.


For the consistency of our codebase, we use eslint with these rules: https://github.com/webcompat/webcompat.com/blob/master/.eslintrc. We also use Prettier for formatting our code. We have set up two recipes to check and fixe the codebase.

`npm run lint` check all JavaScript files and display the resulting errors. If you get errors, you have two possibilities. You can correct yourself or you can run `npm run fix` which use Prettier to format the code.

This comment was marked as abuse.


`npm run lint` check all JavaScript files and display the resulting errors. If you get errors, you have two possibilities. You can correct yourself or you can run `npm run fix` which use Prettier to format the code.

In order to avoid errors during a Pull Request, `npm run lint` will be executed before each commit.

This comment was marked as abuse.

@magsout
Copy link
Member Author

magsout commented May 19, 2017

Done @zoepage

@magsout
Copy link
Member Author

magsout commented May 19, 2017

let me know if it's ok. I'll rebase my commits

@zoepage
Copy link
Member

zoepage commented May 20, 2017

@magsout I'll try to check ASAP. Might take 1-2 days. Sorry!

@zoepage
Copy link
Member

zoepage commented May 31, 2017

Hey @magsout!
Found one tiny thing (I'm sure, I didn't see it before or suggested a wrong fix).

After that, feel free to rebase and merge! Great job! 🎉


If you get an error displayed, there are two ways to fix it.
1. You can run `npm run fix` automatically, which is great for small issues like missing spaces or lines in various files.
2. You can correct it manually as every error message includes the file and line of the error as well as the rule which was violated.

This comment was marked as abuse.

package.json Outdated
"precommit": "lint-staged"
},
"lint-staged": {
"{webcompat/static/js/lib, tests, grunt-tasks}/**/*.js": "lint"

This comment was marked as abuse.

@magsout
Copy link
Member Author

magsout commented Jun 6, 2017

thanks a lot @zoepage for your review. ❤️

@magsout magsout merged commit 603bd54 into master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants