Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eac2a22) 97.26% compared to head (cf2fe6f) 97.26%.

@@           Coverage Diff           @@
##           master     #186   +/-   ##
=======================================
  Coverage   97.26%   97.26%           
=======================================
  Files           2        2           
  Lines         219      219           
=======================================
  Hits          213      213           
  Misses          6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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! 🎉


Thank you for contributing, we are very proud of you! ❤️

@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 👍🏻

@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).

Sorry, I've been away so haven't had a chance. Do feel free to bump in the future, especially if it's blocking!