-
Notifications
You must be signed in to change notification settings - Fork 407
Fix #532, Fix #566, add tslint in ci, add tslint/format/test/karma in precommit of git #565
Conversation
…n git commit hook
…n git commit hook
@@ -22,6 +22,7 @@ before_script: | |||
- ./scripts/sauce/sauce_connect_block.sh | |||
|
|||
script: | |||
- node_modules/.bin/tslint -c tslint.json 'lib/**/*.ts' 'test/**/*.ts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this covered by format:enforce ?
if not what about creating a gulp task ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review, you are right, there already has a task in gulp 'gulp lint', I will use that one.
Pre-commit hook will be executed and following check will | ||
automatically run before commit. | ||
|
||
- tslint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 on creating a commit hook.
Test should run in watch mode anyway ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, test in monitor mode can make sure all jasmine test passed.
But with pre commit, the hook can check format and lint.
Sometimes I forget to do gulp format and gulp lint, and make a commit, the format error was found by Travis, I just want to let developer find the error before commit.
Are you suggesting when code changes, automatically run format and lint check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting when code changes, automatically run format and lint check?
you can set up your IDE/editor for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I see, I have removed the hook
@@ -15,11 +15,19 @@ | |||
}, | |||
"scripts": { | |||
"changelog": "gulp changelog", | |||
"ci": "npm run lint && npm run format && npm run test:single && npm run test-node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document those in the README ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will organize all those scripts and add them into DEVELOPER.md
@vicb, I have updated the PR, and about pre-commit hook, I still keep it because I think we should check lint and format before commit. Please check whether it is a correct way or not. Or maybe we should just add lint and format in pre-commit hook and leave test alone? |
@vicb, I have removed the hook, please review, thank you. |
Fix #532, Fix #566
in git pre-commit with ghooks.
So we can make sure everything is fine before we commit with git.