search

executablebooks / sphinx-tabs

Tabbed views for Sphinx
https://sphinx-tabs.readthedocs.io
MIT License
263 stars 67 forks source link

FIX: unpin docutils #186

Closed agoose77 closed 7 months ago

agoose77 commented 7 months ago

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?

welcome[bot] commented 7 months ago

Thanks for submitting your first pull request! You are awesome! :hugs:
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! :tada:

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #186 +/- ## ======================================= Coverage 97.26% 97.26% ======================================= Files 2 2 Lines 219 219 ======================================= Hits 213 213 Misses 6 6 ``` | [Flag](https://app.codecov.io/gh/executablebooks/sphinx-tabs/pull/186/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | Coverage Δ | | |---|---|---| | [pytests](https://app.codecov.io/gh/executablebooks/sphinx-tabs/pull/186/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | `97.26% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JSS95 commented 7 months ago

This will close #171

agoose77 commented 7 months ago

@foster999 why did you remove the lower bound? I would assume we don't support older docutils.

foster999 commented 7 months ago

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?

agoose77 commented 7 months ago

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

foster999 commented 7 months ago

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?

agoose77 commented 7 months ago

@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 ✨

welcome[bot] commented 7 months ago

Congrats on your first merged pull request in this project! :tada: congrats
Thank you for contributing, we are very proud of you! :heart:

agoose77 commented 6 months ago

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

agoose77 commented 6 months ago

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

foster999 commented 6 months ago

@agoose77 @susnux @edgarrmondragon @JSS95 thanks for your patience, I've just included this in release 3.4.5 👍🏻

foster999 commented 6 months ago

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