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

use pathlib path in sitemap plugin for sphinx compatibility #4215

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

ottojo
Copy link
Contributor

@ottojo ottojo commented Mar 8, 2024

fixes this sphinx warning:

/home/jonas/workspace/ros2_documentation/plugins/sphinx_sitemap_ros.py:228: RemovedInSphinx80Warning: Sphinx 8 will drop support for representing paths as strings. Use "pathlib.Path" or "os.fspath" instead.
  filename = app.outdir + "/" + app.config.sitemap_filename

@ottojo ottojo marked this pull request as ready for review March 8, 2024 21:08
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

I'm somewhat hesitant to put this in as-is. While this definitely works for Sphinx 7 and newer, it does not work for older versions of Sphinx. While that won't affect our deployment (we use Sphinx 7.2.6 currently), it could make the documentation more difficult for people to build locally. If there is a way we can discover the sphinx version and only conditionally do this for sphinx >= 7, that would make me feel better about it.

@ottojo
Copy link
Contributor Author

ottojo commented Mar 13, 2024

it seems to work with sphinx 6 at least, but i added a conditional on sphinx >=7, so even older versions will not be broken by this 👍

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I tested this with Sphinx 7, 6, and 4, and it seems to work with all of them. Thanks!

@clalancette clalancette added the backport-all backport at reviewers discretion; from rolling to all versions label Mar 13, 2024
@clalancette clalancette merged commit 3ec513d into ros2:rolling Mar 13, 2024
4 checks passed
mergify bot pushed a commit that referenced this pull request Mar 13, 2024
* use pathlib paths for sphinx compatibility

* conditionally use pathlib if sphinx >=7

(cherry picked from commit 3ec513d)
mergify bot pushed a commit that referenced this pull request Mar 13, 2024
* use pathlib paths for sphinx compatibility

* conditionally use pathlib if sphinx >=7

(cherry picked from commit 3ec513d)
clalancette pushed a commit that referenced this pull request Mar 13, 2024
…4227)

* use pathlib paths for sphinx compatibility

* conditionally use pathlib if sphinx >=7

(cherry picked from commit 3ec513d)

Co-authored-by: Jonas Otto <[email protected]>
clalancette pushed a commit that referenced this pull request Mar 13, 2024
…4228)

* use pathlib paths for sphinx compatibility

* conditionally use pathlib if sphinx >=7

(cherry picked from commit 3ec513d)

Co-authored-by: Jonas Otto <[email protected]>
@ottojo ottojo deleted the sphinx_path_fix branch March 13, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-all backport at reviewers discretion; from rolling to all versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants