-
Notifications
You must be signed in to change notification settings - Fork 197
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
BUG: Fix serialization with Sphinx 7.3 #1289
Conversation
@maxnoe can you see if the documented changes here make sense to you? |
@lucyleeow ready for review/merge from my end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this, does not look like it was a simple fix. Lots of nits that can be fixed later really.
while working on the same issue at matplotlib i made some changes in gen_gallery and gen_rst to handle the issue with
|
I'll probably try modifying it just to use _get_callables instead so that a callable needs to be passed rather than a class |
Pushed a commit, feel free to give it a try / another look @NirobNabil @lucyleeow ! |
sphinx_gallery/gen_rst.py
Outdated
|
||
|
||
def _get_callables(gallery_conf, key): | ||
# the following should be the case (internal use only) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this means.
Maybe we could add a docstring one liner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it refers to the following assert
line which should always pass because we've ensured it elsewhere
Co-authored-by: Lucy Liu <jliu176@gmail.com>
Co-authored-by: Lucy Liu <jliu176@gmail.com>
Co-authored-by: Lucy Liu <jliu176@gmail.com>
Okay docs hopefully fixed. Realized there were a few to fix in |
Been waiting for this PR to go into a release so that the fix at matplotlib can be merged. Any idea when the next release window of sphinx-gallery might be? |
We'll release after this PR goes in. Edit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked deeply in the tests but should we parametrize some to test config value being str and callable (since we still want to support callable atm). Can be done in separate PR. I guess we would eventually remove callable support if Sphinx moves to toml conf file.
Thanks for fixing this, not an easy one.
@lucyleeow are you up for pushing directly? I think it will be faster |
Can be done in separate PR but what do you think about just updating the |
Done, only changed docs. |
I wish we could but the env (including gallery conf) is cached after SG runs so it would cause the same serializing problems |
Ah I didn't realise that. Can't see a way around that. Feel free to merge! |
Ah I forgot about the tests, I can update in a separate PR. |
Yeah let's get this in and I can start testing with MNE-Python for example. Then maybe release Friday? |
Closes #1286
"FileNameSortKey"
as an alias for"sphinx_gallery.sorting.FileNameSortKey"
sphinx_gallery_conf
clean: don't storeapp
in there or any functionsKind of a pain to figure out but ultimately it does seem cleaner!