Skip to content

Perl hook support#1303

Merged
asottile merged 6 commits intopre-commit:masterfrom
scop:perl
Feb 3, 2020
Merged

Perl hook support#1303
asottile merged 6 commits intopre-commit:masterfrom
scop:perl

Conversation

@scop
Copy link
Copy Markdown
Contributor

@scop scop commented Jan 30, 2020

Here's a first cut for supporting Perl hooks. Appears to work for me with this .pre-commit-hooks.yaml locally added to a https://github.com/perltidy/perltidy clone + the usual config to use it elsewhere:

- id: perltidy
  name: perltidy
  entry: perltidy --nostandard-output --backup-and-modify-in-place
  language: perl
  types: [perl]

No tests yet, haven't taken a look what it would take to add some. Throwing this here for comments already anyway.

@asottile
Copy link
Copy Markdown
Member

neat! looks like a good start -- with some tests should probably be good enough as a v1 implementation!

@scop
Copy link
Copy Markdown
Contributor Author

scop commented Jan 31, 2020

Any hints for getting up to speed with writing the tests as easily as possible? :)

@asottile
Copy link
Copy Markdown
Member

Sure!

For this I think there's ~two main tests you'll want to add:

  1. an integration test which tests a language: perl repository -- you can add a repository in testing/resources/... (there's quite a few examples there) -- and then you'd hook that up with a test in tests/repository_test.py (here's an example for rust)
  2. an integration test demonstrating additional_dependencies -- I'm less familiar with how you'd do this with perl, one easy way might be to define a local-like configuration and ensure that the additional dependency gets installed -- maybe something like this golang test

depending on what's out-of-the-box on azurepipelines we'll probably also need to install perl (?)

@asottile
Copy link
Copy Markdown
Member

asottile commented Feb 1, 2020

if you get stuck I can probably help -- but you'll need to teach me a bit about perl package management :)

@scop
Copy link
Copy Markdown
Contributor Author

scop commented Feb 1, 2020

Thanks, I'll have a look. Additional deps already worked when I tested them -- there are multiple ways one can specify them for cpan to install, but the most usual one is just a module name.

I have no clue about perl in azure, and I'm more familiar with travis anyway, but I don't think it will be a problem at all, just something one needs to add.

One future improvement would be to support repos with cpanfiles in them -- for that one would first make sure either carton (especially if cpanfile.snapshot also exists) or cpanm is installed or install them if not, then proceed with using them. But repos with are less common, let's first get the baseline done.

@scop
Copy link
Copy Markdown
Contributor Author

scop commented Feb 1, 2020

Ok, got those implemented. Linux ok locally and in Azure, but tox_windows py37 fails writing a state file there or something, or maybe it's a result/side effect of something else. I guess it's a fair assumption that perl isn't installed in Windows there, but dunno if it could cause something like that.

I'm afraid I'll need someone to look into this.

@asottile
Copy link
Copy Markdown
Member

asottile commented Feb 3, 2020

I believe I've fixed windows 🤞 -- played around this with a VM -- it also looks like azure pipelines preinstalls "strawberry perl" so perl should already be available

@asottile asottile merged commit 60f30cd into pre-commit:master Feb 3, 2020
@asottile
Copy link
Copy Markdown
Member

asottile commented Feb 3, 2020

🎉 thanks for the patch!

@asottile
Copy link
Copy Markdown
Member

this has been released as part of v2.1.0 -- thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants