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

Enforce tslint in Travis CI? #456

Closed
ascandella opened this issue Jul 16, 2016 · 7 comments
Closed

Enforce tslint in Travis CI? #456

ascandella opened this issue Jul 16, 2016 · 7 comments
Assignees

Comments

@ascandella
Copy link
Contributor

I've noticed some lint violations have snuck in (see unrelated changes in #453).

I don't want to enforce my own beliefs on other projects, but in my experience if lint is "optional" most people are going to ignore it.

Is this intentionally left out of Travis? Is everyone expected to run gulp tslint as part of a pre-commit hook? Am I missing something obvious?

@rebornix
Copy link
Member

@sectioneight I suppose we have tslint in CI as sometimes I see linting failure of my PR.

I always forget the linting stuff as I'm relying on VS Code to do the Git thing so it looks perfect to me to make linting part of pre-commit :)

@jpoon
Copy link
Member

jpoon commented Jul 16, 2016

I think we should be enforcing TSLint. It's run as a part of Travis, but it looks like it's the entire build still succeeds despite the lint errors.

@jpoon jpoon self-assigned this Jul 16, 2016
jpoon added a commit that referenced this issue Jul 16, 2016
@jpoon jpoon closed this as completed in 53fc1a6 Jul 16, 2016
@rebornix
Copy link
Member

Good to see that we have tslint enabled in Travis!

You may also want to put a pre-commit for tslint like @sectioneight mentioned.

#!/bin/sh
pass=true
RED='\033[1;31m'
GREEN='\033[0;32m'
NC='\033[0m'

echo "Running Linters:"

# Run tslint and get the output and return code
tslint=$(gulp tslint)
ret_code=$?

# If it didn't pass, announce it failed and print the output
if [ $ret_code != 0 ]; then
    printf "\n${RED}tslint failed:${NC}"
    echo "$tslint\n"
    pass=false
else
    printf "${GREEN}tslint passed.${NC}\n"
fi

# If there were no failures, it is good to commit
if $pass; then
    exit 0
fi

exit 1

@sectioneight @johnfn @jpoon do you guys think it's necessary to put it the development style guide? Or maybe we can create a Wiki page for collabration.

@jpoon
Copy link
Member

jpoon commented Jul 16, 2016

I've generally relied on running gulp and now that it is enforced with Travis, I feel like that should be sufficient.

I've never played with precommit hooks. Will that script you pasted work cross-platforms (Win/OSX/Linux)?

Update: came across this project which looks kinda neat: https://github.com/therealklanni/git-guppy

@ascandella
Copy link
Contributor Author

I agree that Travis is sufficient. When I wrote this issue, I didn't realize there was a tslint VSCode plugin (which @rebornix's comment hinted me to).

Given the current contribution guidelines (which tell you to run tslint, which is why I opened this issue originally), plus the Travis hooks, I think this is in a good place.

That said, I am a big fan of pre-commit hooks, having gone so far as to develop a framework at a previous company (https://github.com/brigade/overcommit).

@johnfn
Copy link
Member

johnfn commented Jul 16, 2016

@sectioneight you worked at brigade? Neat! I was an intern at Causes once upon a time...

@ascandella
Copy link
Contributor Author

Ha, I actually worked at Causes before brigade (they transferred the repo).
Small world
On Sat, Jul 16, 2016 at 1:43 AM Grant Mathews notifications@github.com
wrote:

@sectioneight https://github.com/sectioneight you worked at brigade?
Neat! I was an intern at Causes once upon a time...


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#456 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAF7P8FbFCaOat44KzvE78WcVQYxkLzJks5qWJm5gaJpZM4JN5ph
.

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

No branches or pull requests

4 participants