add descriptive error message if re.split fails in numpy

[tobiscode]

Hi,
I ran into the same issue as reported in #758, and have created a fork to implement the solution proposed by @dhermangh. In its current form, it prints the affected content and the header it's under, but since the individual docstring conversion function does not know which function/class etc. it is operating on, it can't give that info (or a line number in the original file, for that matter). If there is a way to know the latter, I'm happy to add it as well, but for now, it at least suggest to the user that a type descriptor might be missing.
Does this look good to everyone? Happy to include more fixes.
Cheers,
Tobias

[mhils]

Thanks! I'd suggest we generically catch all exceptions in convert, and then do something like this:

except AnyException:
    raise RuntimeError(f"Docstring processing failed:\n{content=}\n{source_file=}\n{docformat=}") from e

(see pdoc.extract for AnyException - we can move it somewhere else)

[tobiscode]

Hi,

got it - do you want that in addition to the numpy-specific catch block, or instead of? The advantage of keeping both would be that the specific block can point exactly to the part of the docstring that is problematic, which a catch block in convert by itself could not. It might become too verbose for the numpy case, but if I define a NumPyParseException or somthing similar, I could also handle that properly.

Cheers

[mhils]

Instead of. The point of that is to help debugging and fixing pdoc.

I'm happy to merge a second PR that fixes the actual bug in the numpy docstring conversion.

[tobiscode]

that fixes the actual bug in the numpy docstring conversion.

I'm not sure it is a bug - the NumPy docstring convention requires there to be a type for both in- and output parameters. Sphinx has an extension that takes advantage of python type hints but I think it's just for convenience rather than a newer docstring convention. How would you have suggested to fix the parsing bug otherwise?

[mhils]

pdoc.docstrings.convert is not supposed to raise an exception under any circumstances. It may produce a warning, but it must not error.

[tobiscode]

pdoc.docstrings.convert is not supposed to raise an exception under any circumstances. It may produce a warning, but it must not error.

Now I'm even more confused, since in your first comment you suggested to catch AnyException in pdoc.docstrings.convert and then turn it into a RuntimeError. Don't get me wrong, I'm happy to implement whatever solution you prefer, it's just unclear to me what that is.

If you meant pdoc.docstrings.**numpy** is not supposed to raise an exception, then I could modify the re.split operation such that it throws a warning instead. In that case, I would need to know what the code is supposed to do for that section - just ignore it (in which case the Returns section would be empty) or add a dummy variable name (maybe _ in keeping with some Python standards, this would allow the description to still appear in the generated documentation).

[mhils]

Now I'm even more confused, since in your first comment you suggested to catch AnyException in pdoc.docstrings.convert and then turn it into a RuntimeError. Don't get me wrong, I'm happy to implement whatever solution you prefer, it's just unclear to me what that is.

It's not meant to ever throw an exception (but if it does, it should come with useful context for debugging). Phrased differently: If the code has a bug, except AnyException is supposed to attach the full docstring to the traceback for debuggability and then reraise.

If you meant pdoc.docstrings.numpy is not supposed to raise an exception,

Correct, it's not supposed to. If it does, it's a bug. Generally speaking, if something does not parse as a particular format, we should leave the text as-is. I haven't looked super close into the specific numpy case. In either case, that's a separate PR with a test case for more clarity.

Thanks!

[tobiscode]

Okay now I think I get it, thanks for the clarification. What I'll do then is:

1. move the try/except block from the numpy subfunction to the convert function, and update this PR with that commit, and
2. make a minimal test case that for the NumPy-specific case mentioned in pdoc should display which dostring line is causing the issue if a ValueError is raised on re.split #758 produces different test outputs. I'll write that to the issue first, and then after deciding how to handle it, I'll make a new PR.

[mhils]

Perfect, thanks!

[tobiscode]

Okay so it seems the test coverage action fails on these lines:

    except AnyException as e:
        raise RuntimeError(

How should I address this? By making a test case in test_docstrings.py which will raise that exception?

[mhils]

Yes please. The test needs to mock one of the subfunctions to raise. pytest's monkeypatch is great for that.

[tobiscode]

Okay, so since I wasn't familiar with monkeypatch I imitated how the AnyException catches were tested in test_extract.py and did something similar in test_docstrings.py. I hope that's okay, otherwise I can try to do it the monkeypatch way. With that, all tests pass, and the branch could be merged.

There is still a test warning appearing in the logs, however, which seems unrelated to my changes:

 test/test_doc_types.py::test_eval_fail2
  D:\a\pdoc\pdoc\pdoc\doc_types.py:148: UserWarning: Error parsing type annotation xyz for a. Import of xyz failed: name 'xyz' is not defined
    warnings.warn(

It looks to me in doc_types.py that this is one of the possible outcomes, but not the one it's testing for. I don't know how to fix this since I don't understand the logic flow in this test, but happy to make some changes if someone explains it to me. (And probably make a new PR for that.)

[mhils]

This is not a useful test because it tests unwanted behavior (we don't want any known cases of raising). Let's use pytest's monkeypatch to replace rst with a method that raises an exception, and then test with that as you currently do right now (with pytest.raises() is great).

[tobiscode]

Ahhh I see, that makes more sense.

[tobiscode]

Does the new commit look good now?

[mhils]

Thanks! 🍰 😃