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

starting with sphinx 7.2.0, root is a pathlib.Path not a string anymore #1336

Merged
merged 1 commit into from Aug 23, 2023

Conversation

bearsh
Copy link
Contributor

@bearsh bearsh commented Aug 22, 2023

so convert it to a string, the trailing os.sep is not needed. os.path.join which is used to join paths, expects string and inserts the os.sep.

See-also: sphinx-doc/sphinx#11526
See-also: sphinx-doc/sphinx#11608

so convert it to a string, the trailing os.sep is not needed.
os.path.join which is used to join paths, expects string and inserts the
os.sep.

See-also: sphinx-doc/sphinx#11526
See-also: sphinx-doc/sphinx#11608

Signed-off-by: Martin Gysel <me@bearsh.org>
@davvid
Copy link
Member

davvid commented Aug 23, 2023

Thanks! I had just recently pinned sphinx to an older verison because I was running into this bug.

I'll unpin after merging this.

TODO: we should fix this in the upstream sphinxtogithub ~ https://github.com/michaeljones/sphinx-to-github that way it's not just cola that'll benefit from your fix.

davvid added a commit that referenced this pull request Aug 23, 2023
def relative_path() relies on being able to replace values
including the / separator. Keep it there to retain the
original behavior.

Related-to: #1336
Ref: 1293e18
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid davvid merged commit d2a8b9b into git-cola:main Aug 23, 2023
4 of 5 checks passed
davvid added a commit to davvid/sphinx-to-github that referenced this pull request Aug 23, 2023
Newer versions of sphinx use a pathlib.Path object instead
of a string for the "root" parameter. Add compatibility
by stringifying the value provided by Sphinx.

See-also: sphinx-doc/sphinx#11526
See-also: git-cola/git-cola#1336
Original-patch-by: Martin Gysel <me@bearsh.org>
davvid added a commit to davvid/sphinx-to-github that referenced this pull request Aug 23, 2023
Newer versions of sphinx use a pathlib.Path object instead
of a string for the "root" parameter. Add compatibility
by stringifying the value provided by Sphinx.

See-also: sphinx-doc/sphinx#11526
See-also: sphinx-doc/sphinx#11608
See-also: git-cola/git-cola#1336
Original-patch-by: Martin Gysel <me@bearsh.org>
davvid added a commit to davvid/sphinx-to-github that referenced this pull request Aug 23, 2023
Newer versions of sphinx use a pathlib.Path object instead
of a string for the "root" parameter. Add compatibility
by stringifying the value provided by Sphinx.

See-also: sphinx-doc/sphinx#11526
See-also: sphinx-doc/sphinx#11608
See-also: git-cola/git-cola#1336
Original-patch-by: Martin Gysel <me@bearsh.org>
@davvid
Copy link
Member

davvid commented Aug 23, 2023

Nevermind on that TODO -- the upstream project seems to have archived their repo so that it's read-only now.

I pushed a similar commit to my fork ~ https://github.com/davvid/sphinx-to-github

Despite the README saying that it's obsolete, it's still useful when generating docs for a subdirectory of a jekyll site, which is how I'm using it. Thanks for this fix!

@@ -97,7 +97,7 @@ class DirectoryHandler:
def __init__(self, name, root, renamer):
self.name = name
self.new_name = name[1:]
self.root = root + os.sep
self.root = str(root)
Copy link
Member

Choose a reason for hiding this comment

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

I fixed this up post-merge to use str(root) + os.sep because the code outside of the constructor does things like path = directory.replace(self.root, "", 1) (Cf. def relative_path()) which currently relies on the trailing / being there so that it replaces all the way through the /.

It doesn't seem to make a difference in cola's usage, but it doesn't hurt to fix this up in case we end up going down that code path in the future so I tweaked this to keep + os.sep at the end.

davvid added a commit to davvid/git-cola that referenced this pull request Aug 24, 2023
def relative_path() relies on being able to replace values
including the / separator. Keep it there to retain the
original behavior.

Related-to: git-cola#1336
Ref: 1293e18
Signed-off-by: David Aguilar <davvid@gmail.com>
(cherry picked from commit a4b0d27)
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Sep 5, 2023
https://build.opensuse.org/request/show/1108638
by user marcinbajor + anag+factory
- Update to 4.3.2
- Usability, bells and whistles
  * The minimum font size can now be set lower, which is helpful for Hi-DPI displays.
    git-cola/git-cola#1342
- Fixes
  * `git dag` was not displaying history when refspecs were specified.
    git-cola/git-cola#1334
- Development
  * Compatibility with Sphinx 7.2.0 was added to the `sphinxtogithub`
    sphinx documentation plugin.
    git-cola/git-cola#1336
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.

None yet

2 participants