-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
Use pep517 to load project metadata #1311
Use pep517 to load project metadata #1311
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1311 +/- ##
=======================================
Coverage 99.62% 99.62%
=======================================
Files 33 33
Lines 2915 2928 +13
Branches 318 319 +1
=======================================
+ Hits 2904 2917 +13
Misses 5 5
Partials 6 6
Continue to review full report at Codecov.
|
9bb6390
to
86b78c1
Compare
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.
Thanks for the contribution! I've skimmed the PR and have a few comments below.
0323f80
to
a50f480
Compare
Addressed comments, thanks for your review and guidance @atugushev ! |
a50f480
to
3a53732
Compare
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.
Great work! That's a small change with a huge impact. Basically, it provides lock file support for flit. I'm not a maintainer, though, so my opinion means not too much. Is there anything I can help with?
I think we can go ahead and merge this as is. Though, I'm a little worried because we don't have enough tests for |
@atugushev Do you want me to rebase and have a look at the CI? Or will you take care of it? |
@astrojuanlu it's okay, no need to rebase. Could you please take a look at what went wrong? |
@astrojuanlu there is a fix: astrojuanlu#1 |
@atugushev I see that we removed |
ac387d1
to
1e164d5
Compare
Sigh... that made things worse, sorry for the noise. Let me see if I can fix this locally before bothering the CI. |
When running the tests locally with my previous commit, I get this output:
Therefore, tests are only passing on the latest pip version. Now that dependencies get installed with pip through pep517, I think we need to globally disable the pip version check. But I wonder if this is an acceptable thing to do, or to what level of granularity should we control it. |
f765c40
to
1e164d5
Compare
@astrojuanlu I would disable this check globally since it looks not useful for
|
43a372d
to
7ee645d
Compare
Fix #908, first step towards addressing #1047.
Fix #1139.
As per title: this uses pep517 as suggested in pypa/setuptools#1951 (comment) (and mentioned in #908 and #1047).
I did not add any new tests since I did not change any functionality, this is an internal refactor. On the other hand, this is my first contribution to pip-tools so I tried to keep the changes as minimal as possible. If maintainers think that I should also address #1047 in this PR before merging it, I can give it a try.
Changelog-friendly one-liner: Use pep517 to load project metadata
Contributor checklist