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

MNT: Turn the block tuple into a namedtuple #1303

Merged
merged 4 commits into from
May 15, 2024

Conversation

timhoffm
Copy link
Contributor

@timhoffm timhoffm commented May 5, 2024

It's better to give the "Block" concept its own entity and thus be able to access it's attributes by name.

I anticipate that I will need to expand the Block data to support per-block options for #1283, and that expansion will be simpler when blocks are no longer pure tuples. I've decided to split this out for simpler review and advertise this PR as code cleanup.

@timhoffm timhoffm marked this pull request as draft May 5, 2024 21:36
@timhoffm
Copy link
Contributor Author

timhoffm commented May 6, 2024

Not quite sure what the failing test for 3.11 / pip mean. Could anybody hint at whether they are related to this PR or whether that pipeline is broken for other reasons? Thanks.

@lucyleeow
Copy link
Contributor

lucyleeow commented May 7, 2024

I couldn't replicate locally, the rerun timing check has always been a bit brittle but no idea why plot_scraper_broken did not seem to execute in test_error_messages. I've rerun the test 🤷

I think save_figures and docstring of matplotlib_scraper needs to be updated (even though block is never actually used). The scraper docs should be updated too: https://sphinx-gallery.github.io/dev/advanced.html#write-a-custom-image-scraper

@lucyleeow
Copy link
Contributor

Seems spurious, passing now

Copy link
Contributor

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged main in just to make sure everything is still green, good to go from your end @timhoffm ?

@lucyleeow
Copy link
Contributor

I think we have to update scraper.py and scraper docs, but otherwise LGTM!

@timhoffm
Copy link
Contributor Author

timhoffm commented May 14, 2024

Code-wise this is good to go. It's only documentation that could/should be improved. There are a number of places still describing block as tuple. Technically, that is still correct, because a named tuple is a tuple. But describing it as namedtuple would be more helpful.

I'm a bit tight with time now, so it would take a bit until I get around updating the docs as well. If you want to take this as is or update the docs yourself, that would also be very welcome.

@lucyleeow lucyleeow marked this pull request as ready for review May 15, 2024 02:48
@lucyleeow
Copy link
Contributor

Done, @larsoner feel free to merge.

@larsoner larsoner merged commit 85f9a3a into sphinx-gallery:master May 15, 2024
17 checks passed
@larsoner
Copy link
Contributor

Thanks @timhoffm @lucyleeow !

@timhoffm timhoffm deleted the block-tuple branch May 15, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants