-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
sphinx plot directive: sources relative to rst file #12456
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
base: main
Are you sure you want to change the base?
Conversation
This needs at least a note in the api_changes explaining the change and how people relying on the old behavior need to update their code.
|
What happens when you have both an "absolute" path and have also set the |
Uh. I guess the following behavior would have been reasonable, and covers all use cases relatively easily, with a relatively simple upgrade path (just use absolute paths):
But that's neither what was previously used, nor what was previously documented. Moreover, this means that docs won't be buildable with both old and new versions of mpl. (The design proposed in the PR has the same issue I think?) One standard-ish way to handle the transition period would be to introduce a global plot_path_resolution_method = "old" / "new" (names up to bikeshedding) in conf.py and switch the behavior based on that, then we can add deprecation for the "old" method and default to "new". A bit painful, but heh... |
I don't know where to put the API changes text so I'll just post it here for now:
|
I have done the new method here and provided fixes to the broken documentation. I'm not sure if I can implement the deprecation mechanism. |
The api changes note needs to go to this directory: https://github.com/matplotlib/matplotlib/tree/master/doc/api/next_api_changes. I'm fine with the change but let's give other devs the time to chime in especially re: behavior change. |
I am a bit concerned about this, I expect it to break the docs of a fair number of down-stream libraries that use the plot directive as if they are using paths at all they will need to be changed. Atleast scipy and basemap use it (ex
|
I understand and I think having a deprecation period and being able to specify the old and new method is important here (as suggested). |
Not sure if I see this being resolved before 3.1. Not quite sure I understand why this isn't simply a documentation issue... |
@jklymak this is an implementation issue as explained #10350 and we have a fix for it here. The problem is this fix is going to break a lot of packages that depended on this bug (even matplotlib itself). So we'll have to have a deprecation period and support both methods for a while and then remove the old method after a while. Now, I have implemented the new method but was hoping a maintainer would take it from here on and implement the deprecation. |
According to the documentation when plot_basedir is empty or None. The source-code files are found relative to the document that contains them. However, this was not true in the actual implementation. Fixes matplotlib#10350
Absolute paths are found relative plot_basedir which defaults to the source directory. Relative paths are found relative to the document containing them.
@tacaswell Can you make the call regarding the API change? I think the new behavior is nicer, but will be a bit disruptive for downstream projects. |
I agree that an absolute path out to the host file system is probably not what anyone wants. We may want to add a "old-but-dont-warn" option. For down-stream projects they will want to be able to build their docs with a range of mpl versions, at least for a while. Maybe rename the two modes to I am 👍 on moving our docs to use |
How does this interplay (if at all) with #13858? |
Converting to draft. This seems like it will not get fixed, but rather we should properly document the existing behaviour? |
PR Summary
According to the documentation when plot_basedir is empty or None.
The source-code files are found relative to the document that contains
them. However, this was not true in the actual implementation.
Fixes #10350
PR Checklist