Skip to content
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

replace obsolete m2r with sphinx_mdinclude #1449

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

a-detiste
Copy link

  • PR only contains one change (considered splitting up PR)

n2ygk
n2ygk previously approved these changes Aug 6, 2024
Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@n2ygk n2ygk dismissed their stale review August 6, 2024 19:27

rtd build failed

Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@a-detiste
Copy link
Author

It replaces the perfeclty fine docutils by an older one that is too old.

omnilib/sphinx-mdinclude@db37d3f

Downloading docutils-0.18.1-py2.py3-none-any.whl (570 kB)

@a-detiste
Copy link
Author

The pinned libs are too old.

sphinx==7.2.6
sphinx-rtd-theme==1.3.0

@n2ygk
Copy link
Member

n2ygk commented Aug 7, 2024

The pinned libs are too old.

sphinx==7.2.6 sphinx-rtd-theme==1.3.0

So please submit an update commit that pins the appropriate versions. the mdinclude version should also be pinned.

@a-detiste
Copy link
Author

I have absolutely no idea what an "appropriate version" shoud be. I would simply drop pinning altogether & see what happens.

@n2ygk
Copy link
Member

n2ygk commented Aug 7, 2024

I have absolutely no idea what an "appropriate version" shoud be. I would simply drop pinning altogether & see what happens.

The usual approach is to upgrade everything and make sure it works. Then pin those versions (pip freeze).

Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of pinned versions is to avoid future updates that break existing functionality, so a >= test especially for sphinx which has known compatibility issues between major and minor releases. Please:

  1. compare what's in docs/requirements.txt with what's in tox.ini. Both are problematic w.r.t. version pinning as it should only be in one place.
  2. do a tox -e docs to test the sphinx build to make sure everything works as expected.
  3. do a pip freeze and put the appropriate pinned versions into docs/requirements.txt

Thanks.

@dopry
Copy link
Contributor

dopry commented Oct 2, 2024

@a-detiste thanks for your contribution! Do you have time to wrap up this PR? We'd really like to get it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants