-
Notifications
You must be signed in to change notification settings - Fork 27
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
Switch to hatch
for build and (dynamic) versioning
#345
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #345 +/- ##
==========================================
- Coverage 93.59% 93.59% -0.01%
==========================================
Files 18 17 -1
Lines 2342 2341 -1
Branches 520 520
==========================================
- Hits 2192 2191 -1
Misses 85 85
Partials 65 65 ☔ View full report in Codecov by Sentry. |
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.
Hum, there's a lot of stuff I don't get about this PR. We should discuss this one live.
Note that to run tests in all of the supported Pythons, you can:
Or a single specific version:
|
1b7bcd7
to
205b2b7
Compare
hatch
for build and (dynamic) versioninghatch
for build and (dynamic) versioning
5d7bd7a
to
55a8694
Compare
d6a4510
to
070e4d1
Compare
Wow, I just did some digging, and we have never actually directly used The I think It's good to clean this stuff up sometimes! Makes me wonder what other dependency declarations we could safely remove... I suppose |
Not fully reviewed yet, btw, but I've started playing with this. So far I like what I see. I like how the different dependency group declarations are now chained and nicely DRY. |
The drop in coverage shown at https://app.codecov.io/gh/roedoejet/g2p/pull/345/commits tells me we need |
Ah, it seems like it was just misconfigured. Should be okay now. |
Just dropping a reference to juftin/hatch-pip-compile#78 here to explain why |
As you can see there are a couple of bugs (though we are able to work around them) in Poetry apparently has more robust, but quite non-standard, support for dependency groups - I do still prefer |
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.
Sending the comments I wrote but once again I cannot finish my review today, sorry.
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.
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.
OK, finally done my review, and I really like it. I was able to run everything as documented (well, not the hatch-driven matrix test yet, but the rest). With eventlet 0.36.1, tests run and pass. I would declare that dependency as >=0.36.1 now, except for the fact we're about to remove that dependency anyway. So in the mean time I don't care too much.
I'm really looking forward to merging this, I think once AP agree we can go ahead and 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.
In fact, I'm ready to approve this, and we can patch things as needed later.
also switch to using environments for test instead of requirements
You don't actually need to directly use
hatch
for anything but this gives the option to use it for environment management. Also use its version ofsetuptools_scm
to do dynamic versioning based on Git tags. Also use its plugin forpip-compile
to generate a productionrequirements.txt
for Heroku.So, if you want, you can do:
But you can also just: