-
Notifications
You must be signed in to change notification settings - Fork 38
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
Implement --doctest-plus-generate-diff to fix existing docs #227
Conversation
This tries to address gh-107. EDIT: Sorry for failing to run the normal tests on this... Hopefully fixed now. |
Can you use or add to https://github.com/scientific-python/pytest-doctestplus/tree/main/tests ? |
06e4bed
to
cb17e00
Compare
Added one test, checking the very basic modes. I also tried to make sure that we imply So unlike the very first push, I think this now should work and ready for a closer look, which doesn't mean we may not want to reorganize the approach! |
@bsipocz , are you able to review this? I feel like this is going to be very useful when numpy 2.0 is released and people want to update their docs to match. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @seberg!
Minimal wordsmithing and bike-shedding are attached.
The bottom line, I tried this, and it works wonderfully, as expected. I feel the suggestions below make some sense, but feel free to take or leave any of them.
The one more thing that would be nice is a line in the changelog file (CHANGES.rst), but, we certainly can do that at release time.
"folder and use `git diff -p` to generate a diff."), | ||
choices=["diff", "overwrite"], | ||
action="store", nargs="?", default=False, const="diff") | ||
|
||
parser.addini("text_file_format", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether a parser.addini
is needed for this new option, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, that would mean you could specify things as a config file? To some degree, I am not sure I even want that (because I don't really think it is a good habit to run this in CI). But I don't mind adding it either.
pytest_doctestplus/plugin.py
Outdated
"causes editing of the original files.\n" | ||
"NOTE: Unless an in-pace build is picked up, python " | ||
"file paths may point to unexpected places. " | ||
"If `"overwrite"` is not used, will create a temporary " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If `"overwrite"` is not used, will create a temporary " | |
"If 'overwrite' is not used, will create a temporary " |
Just going to use single ' and no backtick.
Thanks for the other suggestions and fixes, they looked great!
Had tested the diff creation, but missed testing the normal operation...
Co-authored-by: Brigitta Sipőcz <b.sipocz@gmail.com>
7e814f3
to
269873e
Compare
Thanks @seberg! |
This implements an approach to allow fixing existing docs (both
.rst
files and inline docs).pytest-doctest
is not always good at guessing where code came from, which makes using this for NumPy unfortunately a bit hard:__module__
which confuses things (the best way to fix this is really to just make the decorator(s) that does this a no-op).As documented, I think this type of tool should always be single-shot use and there should not be any guarantees for it working without any changes.
(In other words, I hope that it is clear that the hooks as implemented here could change!)
It was a while ago that I first wrote this code. @pllim or @bsipocz do you think there is a good way to add some very minimal tests, or do you just want to try it out and see how it goes?
Fix #107