Chore/add linting to pre commit hook

Cambalab / vue-admin

An open source frontend Framework for building admin applications running in the browser on top of REST, using ES6 and Vue.js

GNU General Public License v3.0

132 stars 24 forks source link

Chore/add linting to pre commit hook #183

Closed C3-TKO closed 5 years ago

C3-TKO commented 5 years ago

Description

Added pre commit hook functionality via husky
Added lint staged to lint all javascript files with the npm run lint script prior adding the files to git stage, if lint tests are successful

Closes #158

Type of change

[x] CI environment got enhanced - npm version patch recommended

How Has This Been Tested?

I tried to stage a main.js file with linter violations, e.g. add the following lines somewhere in src/main.js

if (foo) return

Expected result:

Checklist:

The following options in bold are required for a PR approval. Please check the boxes only if necessary, it help us minimizing the reviewing process.

[x] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes

sgobotta commented 5 years ago

@C3-TKO Thanks for submitting this PR! I'll review it soon.

Meanwhile I'm changing the target branch, as stated in the README.md.

sgobotta commented 5 years ago

@C3-TKO could you comment in the related issue claiming you're working on it? That way I can assign it to you and you'll appear in the changelog as contributor, thanks!

sgobotta commented 5 years ago

@C3-TKO could you add support for .vue, .json and.css files in the src/ directory?

It should be something like this: src/**/*.{css,js,json,vue}

Another thing, I can't get it running in my local project. I followed this steps:

Cloned your branch
Run npm install
Write a failing piece of code in src/main.js: if (foo) return
Run git add src/main.js
Run git commit -m "a"
No husky hooks appeared

It must be something in my environment but I have no clue why.

Why do we need another script in the package.json. Is it not enough with npm run lint ?

Environment: npm: 6.10.0 node: v10.16.0

C3-TKO commented 5 years ago

Why do we need another script in the package.json. Is it not enough with npm run lint ?

I have not added any additional script to the packag.json file. However the git diff view might suggest I did. I just added husky and lint staged as dev dependency and added a lint-staged config to the package.json file

C3-TKO commented 5 years ago

@C3-TKO could you add support for .vue, .json and.css files in the src/ directory?

@sgobotta Should lint for CSS not be preferably done it in a more suitable listing tool than es lint?

I will add support for .vue, .json and .css files tomorrow

C3-TKO commented 5 years ago

@sgobotta I will setup my machine (MBPro Mid 2019) with the same node env as the you mentioned and try to install husky and lint-staged agin in order to try to replicate your Report: https://github.com/Cambalab/vue-admin/pull/183#issuecomment-544287172

C3-TKO commented 5 years ago

@sgobotta I noticed that there is only one css source file within this project ./src/assets/fonts/Montserrat/Montserrat.css

Do you really need a linter for a downloaded css font file description? ;-)

This project seems to use sass files

C3-TKO commented 5 years ago

Imho it is easier to include custom fonts via google fonts loader:

<link href="https://fonts.googleapis.com/css?family=Montserrat&display=swap" rel="stylesheet">

sgobotta commented 5 years ago

I noticed that there is only one css source file within this project ./src/assets/fonts/Montserrat/Montserrat.css

Do you really need a linter for a downloaded css font file description? ;-)

This project seems to use sass files

Oh, yeah, you're absolutely right. Let's keep it to .json for config files, .vue for components and .js for the library files.

Imho it is easier to include custom fonts via google fonts loader:

That's true. When we did this we didn't want to rely on Google to do that kind of stuff and have them working without internet connection.

sgobotta commented 5 years ago

@sgobotta I will setup my machine (MBPro Mid 2019) with the same node env as the you mentioned and try to install husky and lint-staged agin in order to try to replicate your Report: #183 (comment)

I'm not sure what could that bee. Followed a few issue sin husky regarding errors in node version above 8.6.0. Tried a few older (< 8.6.0) versions but it didn't work. I'll keep on looking for solutions too.

C3-TKO commented 5 years ago

I installed node in version 10.16.0 along with npm in 6.9.0. Pre-commit hooks were running as intended.

Maybe you could try to run this repo in a node.js docker container.

I changed the config to check for js, json and vue files

sgobotta commented 5 years ago

It seems it was an issue with my git version. Sometimes during npm install, husky can't find the .git//hooks directory and fails to override the hook files header with #husky <version>.

Thanks for your contribution @C3-TKO :tada: :) Changes will be added in 0.0.7.