Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

pre-commit support #2562

Closed
wants to merge 2 commits into from
Closed

pre-commit support #2562

wants to merge 2 commits into from

Conversation

alexjurkiewicz
Copy link

@alexjurkiewicz alexjurkiewicz commented Apr 13, 2017

Overview of change:

Adds a file so that tslint can be used natively with pre-commit. See pre-commit/pre-commit#521 for more discussion.

If this PR is accepted, I'll submit a PR to update the documentation on pre-commit's side.

CHANGELOG.md entry:

[enhancement] pre-commit support


This change is Reviewable

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @alexjurkiewicz! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@alexjurkiewicz
Copy link
Author

There may be some analogous updates from vvakame/typescript-formatter#95 that can be brought into this PR as well (changing language to node and updating the files regex).

@andy-hanson
Copy link
Contributor

andy-hanson commented Apr 13, 2017

Generally tslint should be installed from NPM and not from the repository. Runnable .js files aren't checked-in here, but they are published to NPM.

@adidahiya
Copy link
Contributor

@alexjurkiewicz can you provide an overview of how this works so that I don't have to comb through the pre-commit documentation? As @andy-hanson noted above, there are no ready-to-go Node.js executables checked in here on Github.

@alexjurkiewicz
Copy link
Author

Quick description of pre-commit: pre-commit is a tool that runs different scripts (linters, formatters, etc) on your files-to-be-committed based on their file extension, giving a quick pass/fail for simple code quality tests / other uses. The addition of a .pre-commit-hooks.yaml file to this repo allows pre-commit users to use tslint with an official configuration (eg no dependence on third party repo). Here's an example terminal session:

$ cat .pre-commit-config.yaml
# If this PR is changed, the contents would change to refer to the official tslint repository
- repo: local
  hooks:
  - id: tslint
    name: tslint
    entry: tslint
    files: \.tsx?$
    language: node

$ pre-commit install
pre-commit installed at /home/aj/tsrl/.git/hooks/pre-commit

$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   src/entity-list.ts

$ git commit
tslint...................................................................Failed
hookid: tslint

ERROR: src/entity-list.ts[11, 5]: Calls to 'console.log' are not allowed.
ERROR: src/entity-list.ts[12, 5]: Calls to 'console.log' are not allowed.

$

Generally tslint should be installed from NPM and not from the repository. Runnable .js files aren't checked-in here, but they are published to NPM.

This PR isn't quite correct, but the idea is pre-commit will create an empty temp directory, run npm install tslint and then tslint modified/file.ts. So there doesn't need to be anything in this repository except .pre-commit-hooks.yaml file and package.json. You could create a blank stub repo like jshint instead. The value in putting the file in this repo is conceptual simplicity for users (refer to the official tslint repo to use tslint).

@adidahiya
Copy link
Contributor

Can pre-commit users pin to a specific version of TSLint or does it always use the HEAD of the default branch?

@alexjurkiewicz
Copy link
Author

The SHA key in the above snippet pins to a specific version of this repo's pre commit hooks file. If the file is periodically updated with the latest version of tslint as an additional dependency, users can pick a sha with the desired tslint version.

Technically, you can also use a tag. So if you release tslint v13, before you tag the repo with v13, just update the pre commit file. Then users can specify this repo@v13 to get their desired version.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants