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

Implement --doctest-plus-generate-diff to fix existing docs #227

Merged
merged 10 commits into from
Dec 9, 2023

Conversation

seberg
Copy link
Contributor

@seberg seberg commented Oct 6, 2023

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:

  1. NumPy overrides __module__ which confuses things (the best way to fix this is really to just make the decorator(s) that does this a no-op).
  2. NumPy has a lot of C-functions and there is absolutely no way to guess where their docstrings are.

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

@seberg
Copy link
Contributor Author

seberg commented Oct 6, 2023

This tries to address gh-107.

EDIT: Sorry for failing to run the normal tests on this... Hopefully fixed now.
EDIT2: OK, clearly there was a bit more to this, another fixup (can't be quite sure it is the last)

@pllim pllim added this to the 1.1.0 milestone Oct 6, 2023
@pllim
Copy link
Contributor

pllim commented Oct 6, 2023

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?

Can you use or add to https://github.com/scientific-python/pytest-doctestplus/tree/main/tests ?

README.rst Outdated Show resolved Hide resolved
@seberg
Copy link
Contributor Author

seberg commented Oct 6, 2023

Added one test, checking the very basic modes. I also tried to make sure that we imply --doctest-only (which I think works).
The test seems to run in the same Python session, but I use globals (is there a way to avoid that?), so I simply reset the reporting to not report it on a normal test run, which seems wrong, but printing out info about changed files when running pytest on doctestplus isn't better.

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!

@pllim pllim requested a review from saimn October 7, 2023 01:36
@pllim
Copy link
Contributor

pllim commented Nov 28, 2023

@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. 🙏

Copy link
Member

@bsipocz bsipocz left a 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.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
pytest_doctestplus/plugin.py Outdated Show resolved Hide resolved
pytest_doctestplus/plugin.py Outdated Show resolved Hide resolved
pytest_doctestplus/plugin.py Outdated Show resolved Hide resolved
pytest_doctestplus/plugin.py Show resolved Hide resolved
"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",
Copy link
Member

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?

Copy link
Contributor Author

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.

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

Choose a reason for hiding this comment

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

Suggested change
"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!

@bsipocz bsipocz merged commit 5216f24 into scientific-python:main Dec 9, 2023
17 checks passed
@bsipocz
Copy link
Member

bsipocz commented Dec 9, 2023

Thanks @seberg!

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.

Provide an option to update an rst page with expected output
3 participants