Ensure doctests do not have side effects#17747
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
| >>> for format_name in ['custom_no_header', 'custom_commented_header', 'fixed_width_commented_header']: | ||
| ... registry.unregister_reader(f"ascii.{format_name}", Table) | ||
| ... registry.unregister_writer(f"ascii.{format_name}", Table) | ||
| ... del FORMAT_CLASSES[format_name] |
There was a problem hiding this comment.
@taldcroft - it is probably not intended that format classes stay around even if you unregister the reader and the writer. It may not matter too much in practice, but without removing it, I get a failure in
astropy/astropy/table/tests/test_mixin.py
Lines 111 to 123 in 11aa662
Note that this uses a private method, so a user might never hit this problem.
There was a problem hiding this comment.
The io.ascii FORMAT_CLASSES is formally independent of registration as a Table reader/writer since users may be calling io.ascii.read(...). In the current paradigm, unregistering should not imply removal from FORMAT_CLASSES.
In theory we could decide to make io.ascii only be providing connect functions for unified I/O, and not provide any public API. But that would be a lot of churn without really helping all that much.
There was a problem hiding this comment.
OK, makes sense. I wondered about exposing some of this (also the units registry list), but think in the end it is not super useful and certainly not urgent: in general, I think people are pretty used to restarting python if something really gets messy with registered classes, etc.
|
Double test passed! |
| >>> for format_name in ['custom_no_header', 'custom_commented_header', 'fixed_width_commented_header']: | ||
| ... registry.unregister_reader(f"ascii.{format_name}", Table) | ||
| ... registry.unregister_writer(f"ascii.{format_name}", Table) | ||
| ... del FORMAT_CLASSES[format_name] |
There was a problem hiding this comment.
The io.ascii FORMAT_CLASSES is formally independent of registration as a Table reader/writer since users may be calling io.ascii.read(...). In the current paradigm, unregistering should not imply removal from FORMAT_CLASSES.
In theory we could decide to make io.ascii only be providing connect functions for unified I/O, and not provide any public API. But that would be a lot of churn without really helping all that much.
neutrinoceros
left a comment
There was a problem hiding this comment.
Thank you ! I agree that it's worth doing, and I'm glad I learned about pytest-doctestplus's testcleanup because of it.
I can also confirm this does not fix #17745 (though that's to be expected), but it also doesn't make it worse, so let's get this in !
|
Thanks! I was gonna backport my other PR. Let's see... @meeseeksdev backport to v7.0.x |
…747-on-v7.0.x Backport PR #17747 on branch v7.0.x (Ensure doctests do not have side effects)
… (Ensure doctests do not have side effects)
In #17732, @pllim found that a double run of both the regular tests and the doctests caused failures. While in principle we should not necessarily be as strict for the doctests that there are no side effects, I thought we might as well clean it up. So, I did, and also picked @pllim's first commit from #17743 so that it gets tested.
Note may not fix #17745, as I haven't tied loading all the dependencies. One step at a time, I guess! (cc @neutrinoceros)
closes #17743 (since this PR uses its first commit that also includes the positional arguments)