Skip to content

Added the additional_dependencies config parameter - #295

Merged
asottile merged 9 commits into
pre-commit:masterfrom
blarghmatey:install_dependencies
Nov 23, 2015
Merged

Added the additional_dependencies config parameter#295
asottile merged 9 commits into
pre-commit:masterfrom
blarghmatey:install_dependencies

Conversation

@blarghmatey

@blarghmatey blarghmatey commented Nov 19, 2015

Copy link
Copy Markdown
Contributor

Added the ability to specify additional dependencies to be installed in
the pre-commit environment.

@blarghmatey
Added the additional_dependencies config parameter
06b3d91 
Added the ability to specify additional dependencies to be installed in
the pre-commit environment. Fixed broken tests.
@blarghmatey

blarghmatey commented Nov 19, 2015

Copy link
Copy Markdown
Contributor Author

I'm opening this pull request now while I add tests in order to get some feedback before going much further with it.

asottile
asottile reviewed Nov 19, 2015
View reviewed changes
Comment thread pre_commit/languages/node.py Outdated

@asottile asottile Nov 19, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor nit. I'd format this like:

def install_environment(
    repo_cmd_runner,
    version='default',
    additional_dependencies=(),
):

This minimizes merge conflicts slightly when adding / removing / reordering parameters

@asottile

asottile commented Nov 19, 2015

Copy link
Copy Markdown
Member

Looks great, don't worry too much about appveyor, their stuff is broken right now :/

asottile
asottile reviewed Nov 19, 2015
View reviewed changes
Comment thread pre_commit/languages/python.py Outdated

@asottile asottile Nov 19, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just thought about this, these'll need quoting in the general case:

env.run(
    'cd '{{prefix}}' && '
    'pip install {deps}'.format(
        ' '.join(pipes.quote(dep) for dep in additional_dependencies),
    ),
)

@asottile asottile Nov 19, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

errr, pre_commit.util.shell_escape instead of pipes.quote

@blarghmatey

blarghmatey commented Nov 19, 2015

Copy link
Copy Markdown
Contributor Author

Any advice on where I should be looking at for adding tests to address the coverage issue on 2.6?

@asottile

asottile commented Nov 19, 2015

Copy link
Copy Markdown
Member

@blarghmatey you'll want to add a test that triggers the additional_dependencies behaviour for each of the three languages (the coverage it is complaining about).

py26 is notably touchy due to some coverage bugs around context managers that are hard to avoid / factor out (which is why the minimum is set to 99% anyway).

You can probably add the tests to tests/repository_test.py. You'll want to augment def _test_hook_repo to support additional_dependencies I think? Or use the bits that that guy calls in order to write your own helper.

I also sent a PR for you: blarghmatey#1

:)

@blarghmatey
blarghmatey force-pushed the install_dependencies branch 3 times, most recently from 13898e4 to e96e1a2 Compare November 20, 2015 20:00
@blarghmatey

blarghmatey commented Nov 20, 2015

Copy link
Copy Markdown
Contributor Author

@asottile Can you duplicate the test failures on your machine? They run successfully on my laptop so I'm not exactly sure how to fix them.

@asottile

asottile commented Nov 20, 2015

Copy link
Copy Markdown
Member

Yep, I'll run through these :)

@asottile

asottile commented Nov 20, 2015

Copy link
Copy Markdown
Member

I opened a PR against your branch: blarghmatey#2

@asottile

asottile commented Nov 20, 2015

Copy link
Copy Markdown
Member

One last thing, let's add these to the json schemas (pre_commit/clientlib/validate_{manifest,config}.py

And then :shipit:!

@blarghmatey

blarghmatey commented Nov 23, 2015

Copy link
Copy Markdown
Contributor Author

@asottile Hope you had a good weekend! I added the addtional_dependencies attribute to the config schema. I didn't add it to the manifest schema because it didn't seem relevant in that context since the setup.py, package.json, etc. files should handle the dependencies in those repos. Happy to add it in if you think it should be there.

@asottile

asottile commented Nov 23, 2015

Copy link
Copy Markdown
Member

@blarghmatey sounds good. I'll merge this when I get into work (I'll probably spec them in manifest anyway since I have some usecases where I'll want them there)

Thanks for the awesome PR! Please add your self (if you'd like) on https://github.com/pre-commit/pre-commit.github.io

@blarghmatey

blarghmatey commented Nov 23, 2015

Copy link
Copy Markdown
Contributor Author

@asottile Awesome! I'll also add in the new attribute to the docs on the site.

asottile added a commit that referenced this pull request Nov 23, 2015
@asottile
Merge pull request #295 from blarghmatey/install_dependencies
7c8272d 
Added the additional_dependencies config parameter
@asottile
asottile merged commit 7c8272d into pre-commit:master Nov 23, 2015
@asottile asottile mentioned this pull request Nov 23, 2015
Closed
@asottile asottile mentioned this pull request Dec 11, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@blarghmatey @asottile