search

executablebooks / MyST-NB

Parse and execute ipynb files in Sphinx
https://myst-nb.readthedocs.io
BSD 3-Clause "New" or "Revised" License
211 stars 84 forks source link

Fix image metadata being used in wrong cell #599

Closed flying-sheep closed 4 months ago

flying-sheep commented 6 months ago

Fix bug pointed out in https://github.com/executablebooks/MyST-NB/pull/588#pullrequestreview-2040630500 caused by reusing the image_options dict. Fixes #605.

Seems like in my previous PR I didn’t understand the tests enough to realize that I changed them to nonsense, but now I do.

Sorry for that!

cc @OriolAbril

juanitorduz commented 5 months ago

Hi! It would be great to have this fix into the next release 😄.

agoose77 commented 5 months ago

Thanks for your feedback everyone! I am doing a round of maintenance on the Sphinx stack at the moment, I will aim for a release next week.

flying-sheep commented 5 months ago

Hi @agoose77 are you still planning to do a release soon?

agoose77 commented 4 months ago

Thanks @flying-sheep, this is very helpful. Another contributor also opened a PR https://github.com/executablebooks/MyST-NB/pull/609, so I'm in the fortunate position of having to make a choice between the two! As there's a new test in #609, I'm going to close this in favour of that one :)

OriolAbril commented 4 months ago

@agoose77 I would merge both PRs actually. They do have the addition of the .copy() in common, but that is all, so I would not expect merge conflicts either. This PR fixes the existing tests ehich are as of now testing that the wrong behaviour happens (consequently, all tests are failing on the other PR). The other PR doesn't change anything about existing tests (which needs to happen too in addition to fixing the bug) and adds a new test that is more specific for image metadata.

If you prefer, I can also merge both PRs into one keeping the commits of each author so contributions are properly tracked and open a new with the changes from both PRs

agoose77 commented 4 months ago

@OriolAbril yep, I'm pulling the changes from the tests to attribute @flying-sheep :)