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

Fix slice assignment in update_context() #181

Merged
merged 1 commit into from Oct 22, 2023

Conversation

mathpl
Copy link
Contributor

@mathpl mathpl commented Oct 21, 2023

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
Copy link

welcome bot commented Oct 21, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

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! 🎉

@foster999
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

mathpl commented Oct 21, 2023

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

@mathpl
Copy link
Contributor Author

mathpl commented Oct 22, 2023

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

@foster999
Copy link
Collaborator

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

@foster999 foster999 merged commit 973a049 into executablebooks:master Oct 22, 2023
5 of 8 checks passed
@welcome
Copy link

welcome bot commented Oct 22, 2023

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

@mathpl
Copy link
Contributor Author

mathpl commented Oct 22, 2023

Glad to help and thank you for the extension!

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