FIX: unpin docutils #186
FIX: unpin docutils #186
Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #186 +/- ##
=======================================
Coverage 97.26% 97.26%
=======================================
Files 2 2
Lines 219 219
=======================================
Hits 213 213
Misses 6 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This will close #171 |
|
@foster999 why did you remove the lower bound? I would assume we don't support older docutils. |
|
Most of the changes to sphinx, pygments and docutils have no functional impact on the package so I reckon it should be safe to drop it Happy to merge it if that sounds alright? |
|
@foster999 just want to clarify the lower bound - this just means old docutils aren't allowed. I think that's fair - we know that docutils has changed in the last N years, and would break if we used it. I don't know if 0.18 is the oldest version that technically works, but it's old enough that I don't think it matters. So, I think it's better to keep a lower bound, as the argument is different to that of the upper bound. I'm in favour of removing the upper bound because we can't predict whether a future version of docutils will break us. |
|
I totally appreciate this, but there's some extra context to the current pin.The current pin is a legacy from when the dependency pins reflected which versions our tests covered. Our regression tests are too non-specific, so non-functional changes to the dependencies often break them. This was painful, because we had to update the pin to adopt new versions (like in this situation). We now only test against the latest versions of these dependencies, so just the teats need updating each time So I don't think the current pin is a fair reflection of the minimum version that the package works with. I'd rather drop it than have an arbitrary value. And this approach has been fine for sphinx and pygments so far Apologies, I should have provided a better explanation when I suggested the change. I hope this makes sense? |
|
@foster999 thanks for the extra context! It might be useful to lower-bound sphinx rather than docutils, but I'll leave that suggestion for future consideration! LGTM ✨ |
|
Congrats on your first merged pull request in this project! 🎉 |
|
@foster999 could we make a release with this PR included? I can do this myself if you don't have the bandwidth :) I'd probably make a patch release, as our APIs won't be changing (unless some other PR has changed us). |
|
@foster999 another friendly ping (hoping you are well?) — do you have any idea on when you might have the bandwidth to look at cutting a release? We depend upon this package in JB's test suite, so it would be nice to release a new version. |
|
@agoose77 @susnux @edgarrmondragon @JSS95 thanks for your patience, I've just included this in release 3.4.5 👍🏻 |
Sorry, I've been away so haven't had a chance. Do feel free to bump in the future, especially if it's blocking! |
Unless we know that docutils is broken for some version, and will be in future, we should unpin here so that downstream users are not blocked.
AFAICT, there's no strong reason in this package to pin?
@foster999 are you OK with this change?