-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Pipfile support (schema) #4782
Pipfile support (schema) #4782
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm blocking this one because the YAML is important and I want to be sure that I'm understanding the changes proposed here before I approve it.
In particular, the options --skip-lock
and --ignore-pipfile
are kind of confusing after reading the documentation. Using them together doesn't make too much sense and seems that pipenv should fail.
assertValidConfig(tmpdir, content.format(value=value)) | ||
|
||
|
||
@pytest.mark.parametrize('value', [True, False]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True
and False
shouldn't be true
and false
since it's a YAML file?
Other tests use true
and false
, which I think it's correct.
@@ -77,6 +77,22 @@ python-install: | |||
# Default: [] | |||
extra_requirements: list(str(), required=False) | |||
|
|||
python-install-pipfile: | |||
# The path to the project to be installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be,
The path to the directory that contains the Pipfile to use
since the project is not going to be installed, but all the dependencies on the Pipfile.
Codecov Report
@@ Coverage Diff @@
## master #4782 +/- ##
=========================================
+ Coverage 76.12% 76.22% +0.1%
=========================================
Files 158 158
Lines 10007 10025 +18
Branches 1261 1264 +3
=========================================
+ Hits 7618 7642 +24
+ Misses 2044 2039 -5
+ Partials 345 344 -1
|
@agjohnson @humitos this is ready for re-review, I have changed (inverted) the defaults as #4782 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I think the schema matches how we want to use pipfile by default. I'm excited to get this implemented!
I think this satisfies the all the feedback, so we can unblock this. If @humitos has no other feedback tihs is ready for merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks 👍
@@ -330,17 +330,38 @@ def test_python_install_extra_requirements_empty(tmpdir, value): | |||
assertValidConfig(tmpdir, content.format(value=value)) | |||
|
|||
|
|||
def test_python_install_pipfile(tmpdir): | |||
@pytest.mark.parametrize('pipfile', ['another_docs/', '.', 'project/']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Ref #3181