search

spatialaudio / nbsphinx

:ledger: Sphinx source parser for Jupyter notebooks
https://nbsphinx.readthedocs.io/
MIT License
450 stars 130 forks source link

Handle doc2path() API change #807

Closed mgeier closed 1 month ago

mgeier commented 1 month ago

Hopefully fixes #804.

flying-sheep commented 1 month ago

seems all a bit hacky.

I think the idea of the deprecation is that Sphinx eventually wants to return pathlib paths from these APIs.

So maybe a better fix would be to wrap the instances of these that represent concrete filesystem paths in pathlib.Path and using its APIs instead of string manipulation. But I didn’t take a deeper look here.

mgeier commented 1 month ago

It sure is hacky, but contrary to the Sphinx maintainers, I value backwards compatibility, and I am somewhat confident that my PR here will continue to work with old versions (as well as new versions), while if I switch everything to pathlib, it will break when using old Sphinx versions, right?

flying-sheep commented 1 month ago

no I mean wrapping the return values of that API in Path(...)

then you don’t need to use string operations on paths anymore

callezenwaka commented 1 month ago

Hey @mgeier,

Is this PR getting merged soon? I have a failing pipeline and do not want to filterwarnings the error.

mgeier commented 1 month ago

@callezenwaka

Is this PR getting merged soon? I have a failing pipeline and do not want to filterwarnings the error.

Yes, I'm planning to merge this soon, I just wanted to respond to @flying-sheep's comments first.

It would be great if you could test this on your project(s) and report if it works or not!

mgeier commented 1 month ago

I'm going to merge this now, because people seem to be waiting for a fix, but I'm still open for changes which I can incorporate at a later time.

flying-sheep commented 1 month ago

Thanks for responding! Sorry for holding this up with my curiosity!

mgeier commented 1 month ago

No worries, I like getting feedback!