search

executablebooks / sphinx-tabs

Tabbed views for Sphinx
https://sphinx-tabs.readthedocs.io
MIT License
263 stars 67 forks source link

Fix slice assignment in update_context() #181

Closed mathpl closed 9 months ago

mathpl commented 9 months ago

I have to be honest, I'm not familiar with slice assignment like these, however inspired by https://github.com/sphinx-doc/sphinx/blob/v7.1.2/sphinx/builders/html/__init__.py#L1071-L1073 I think we may have flipped it here. Based on my testing it no longer strips script_files from other extensions while still properly removing tabs.js when it's not used.

welcome[bot] commented 9 months ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.
Welcome to the EBP community! :tada:

foster999 commented 9 months ago

Thanks for this @mathpl!

Slicing (all or part of a builtin object) creates a shallow copy, instead of creating a reference to the original one in memory. I'm not clear on what it does on the left side of assignment though 🙃 I'll have a try and see.

So does this change stop our extension from breaking a py-data-theme site? And do you have an example I could build to see this please?

And is the theory that this code was removing some of py-data-theme's JS? I'd be keen to understand the root cause

foster999 commented 9 months ago

So when I slice on the left side of assignment the object is copied to a new location in memory, and all of its references are updated to point to the new location. When I slice on the right side other references still point to the original location in memory.

I'm not sure why that difference would cause a problem, as any extension run before or after ours would pick up the latest context object from sphinx.

mathpl commented 9 months ago

I'll try to get you a repro setup so you can play around with it. Thanks!

mathpl commented 9 months ago

🎁 https://github.com/mathpl/sphinx-tabs-repro

foster999 commented 9 months ago

Thanks for the example 😄I haven't been able to work out what the odd slicing is doing, but can reproduce that the fix works. I'll get a new release out asap after I fix the breaking tests in #179

welcome[bot] commented 9 months ago

Congrats on your first merged pull request in this project! :tada: congrats
Thank you for contributing, we are very proud of you! :heart:

mathpl commented 9 months ago

Glad to help and thank you for the extension!