PRs to add messages to assertion errors or replace with TypeError, ValueError, etc?

[Jacob-Stevens-Haas]

Intercepting sphinx's errors is difficult (#10670). Reading sphinx's error messages is also difficult, since many come from assert statements. E.g. the error here:

Exception occurred:
  File "/home/jake/github/pysindy/env/lib/python3.9/site-packages/sphinx/writers/html5.py", line 321, in visit_reference
    assert len(node) == 1 and isinstance(node[0], nodes.image)  # NoQA: PT018
AssertionError
The full traceback has been saved in /tmp/sphinx-err-mfunnqy9.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.

is generated from this code. Would PRs to add a message (e.g. "reference nodes must have a TextElement (such as paragraph) as parent or contain an image node as the sole child") be welcome? Would converting the exception type (here to a ValueError) be welcome, or were these made assertions intentionally?

[AA-Turner]

Thanks for the suggestion! Ideally the assumptions leading to the assert would be updated or fixed, though this might not be possible.

Perhaps we could change assert x to if not x: raise AssertationError(msg)? That would keep the exception type, and also use an informative message.

A

[mgeier]

Different people have different opinions what assert means and what it should mean. Here's mine:

• users should ideally never see an AssertionError
• AssertionErrors are caused by bugs, not by wrong user input
• when a user sees an AssertionError, they have done nothing wrong, and there is nothing they can do except inform the maintainers (ideally with a minimal reproducible example)
• there are 2 kinds of bugs related to this:
  • there's a "real" bug which should be fixed to avoid reaching the assert in the future
  • the assert itself is the bug, and should rather be converted into a different (user-facing) exception (as suggested above)

we could change assert x to if not x: raise AssertationError(msg)?

No, that would be bad.

The correct syntax is this:

assert len(something) == 1, "only one thing is allowed"

See https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement.

[timhoffm]

I support @mgeier's comment above.

Genreally, I believe there should only be very few asserts in the code (outside of tests of course).

I've noticed that there are quite a few IMHO unnecessary guards for programming errors. These two asserts are basically unnecessary

sphinx/sphinx/writers/latex.py

Lines 1362 to 1365 in eb337fe

	assert self.table is not None
	cell = self.table.cell()
	assert cell is not None
	self.table.col += cell.width

If they are not there, the program will fail in the following line with "Attribute error: None object has no attribute table/width", which IMHO is almost as informative as the existing assertion in the line above. I would leave the assertions out here to make the code more readable.