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

Add pre-commit support #95

Closed
wants to merge 2 commits into from
Closed

Add pre-commit support #95

wants to merge 2 commits into from

Conversation

alexjurkiewicz
Copy link

This PR adds a file so that tslint can be used natively with pre-commit.

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

Copy link
Owner

@vvakame vvakame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contributions!
I first learned about the pre-commit project now 😉
I have a some questions.

description: TypeScript code auto-formatter.
entry: tsfmt
args: ['--verify']
language: system
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use system?
Is node more appropriate? http://pre-commit.com/#node

My suggestions, We should use project local tsfmt, not system wide tsfmt.
I think you have a two or more project about tsfmt using. There project should be able to update dependencies independently.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. I haven't used node language before, but it seems like tsfmt should be compatible as-is.

entry: tsfmt
args: ['--verify']
language: system
files: \.ts$
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the regexp?
I want to use \.tsx?$.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@vvakame
Copy link
Owner

vvakame commented Apr 13, 2017

👀 palantir/tslint#2562

@vvakame
Copy link
Owner

vvakame commented Apr 14, 2017

I tried to use pre-commit, but I can't do it..
https://github.com/vvakame/typescript-formatter/tree/alexjurkiewicz-pre-commit
https://gist.github.com/vvakame/3ae719362d4de86b966ae6f79deb0d58

I have a question.
tsfmt & tslint projects are must be installed via npm.
Is pre-commit project can do that? I think that that project is not support it well...

@alexjurkiewicz
Copy link
Author

Sorry for the error. I thought I understood pre-commit + node but I didn't 🙇 .

The solution is to add an additional_dependencies key to .pre-commit-hooks.yaml:

-   id:          tsfmt
    name:        TypeScript Formatter (tsfmt)
    description: TypeScript code auto-formatter.
    entry:       tsfmt --verify
    language:    node
    files:       \.tsx?$
    additional_dependencies: ['typescript-formatter', 'typescript']

You can also specify a certain version of dependencies like using npm/yarn:

additional_dependencies: ['typescript-formatter', 'typescript@2.2.2']

However, this will mean that there would need to be a new commit for each typescript version that changes typescript-formatter behaviour. Each update to the pre commit file could be recorded for users to use a certain version of typescript with. Example .pre-commit-hooks.yaml content:

# Use the following SHAs:
# * typescript 1.9: aaaaaaaa
# * typescript 2.0: bbbbbbb
# * typescript 2.2: cccccccc
-   repo: git@github.com:vvakame/typescript-formatter.git
    sha: <sha>
[...]

This is getting a bit complex, sorry. I think you wouldn't like this in your main repo, is that right?

What I can do instead is maintain a dedicated pre-commit-tsfmt repository, like jshint.

@alexjurkiewicz
Copy link
Author

One possible additional complication. The additional_dependencies line should probably specify the typescript-formatter version.

@vvakame
Copy link
Owner

vvakame commented May 3, 2017

sorry for late reply.

I think you wouldn't like this in your main repo, is that right?
What I can do instead is maintain a dedicated pre-commit-tsfmt repository, like jshint.

sounds good! I recommended it.
I am not a user, so it would be better to maintain it by the people actually using it.

@vvakame vvakame closed this Jul 25, 2017
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

Successfully merging this pull request may close these issues.

None yet

2 participants