False positive DOC503 in 0.5.12

[Waschenbacher]

False positive DOC503 error in version 0.5.12?

------------------------Bug report---------------------
[Example]:

def check_nonnegative_bug(number: float) -> None:
    """Check if the provided number is non-negative.

    Args:
        number(float): The number to check.

    Returns:
        None

    Raises:
        ValueError: when the number is less than zero.
        ValueError: when the number is zero.
    """
    if number < 0.0:
        raise ValueError("The number must be positive.")
    if number == 0.0:
        raise ValueError("The number must be positive.")

[To reproduce]
Running pydoclint --style=google --check-return-types=False . on terminal in a conda env (Env details see below) gives the pydoclint error message below.

[pydoclint error message]

DOC503: Function check_positive exceptions in the "Raises" section in the docstring do not match those in the function body Raises values in the docstring: ['ValueError', 'ValueError']. Raised exceptions in the body: ['ValueError'].

[Expected output]

• no violations

[Environment]

• pydoclint 0.5.12 pypi_0 pypi
• python 3.10.16 h870587a_1_cpython conda-forge

------------------------Good example---------------------
My guess: It seems that the false alarm on DOC503 occurs only when there are multiple errors of the same type. For errors of different types (see the example below), no pydoclint complaints.

[Example]:

def check_nonnegative_ok(number: float) -> None:
    """Check if the provided number is non-negative.

    Args:
        number(float): The number to check.

    Returns:
        None

    Raises:
        TypeError: when the type of number is not float.
        ValueError: when the number is less than zero.
    """
    if not isinstance(number, float):
        raise TypeError("The type of number must be float.")
    if number <= 0.0:
        raise ValueError("The number must be non-negative.")

[Output]:

• no violations

[Other details]:
All other details are the same as in the bug-example.

[jsh9]

Hi @Waschenbacher , this is not a bug, but a feature.

Please see the code here:

pydoclint/pydoclint/utils/return_yield_raise.py

Lines 98 to 100 in dfd9494

	def getRaisedExceptions(node: FuncOrAsyncFuncDef) -> list[str]:
	"""Get the raised exceptions in a function node as a sorted list"""
	return sorted(set(_getRaisedExceptions(node)))

We deduplicated and sorted the exceptions within a function. The reason is that very often we have functions with many exceptions (e.g., 5 TypeErrors, 3 ValueErrors, 2 IOErrors, ...), so it's not very practical to list every raised exception in the correct order in the docstring. (Note: if you don't want deduplication, the only sensible for a linter is to enforce the order.)

It's not difficult to add another config option to suppress deduplication, but the key question is whether we should add such an option. My current position is no, unless there are very compelling reasons.

[Waschenbacher]

Hi @jsh9
thanks for the elaboration. We were using an older version and switching to the latest version raises many errors. Now it makes sense.

[s-weigand]

I just stumbled on the same thing (also 2 ValueError's 😅) after an update.
I would love to have the afore mentioned suppress deduplication option (I could also make a PR so @jsh9 has less work himself).

My reasoning (hopefully compelling) is simply how it renders in sphinx.
Having a list style renders as a list.

My naive approach to fix the linter issue was to just or separate the reasons for the ValueError, which looks fine in a hover info render.

However sphinx gets rid of duplicated newlines quite aggressively which makes the docs harder to read.

While one can consider having multiple errors of the same type a bad thing for handling them up the line, there are also quite a few cases where you do not want to handle errors but let it error out and still give users a fine grain actionable error message, without the need to create a subclass for each error reason.

[jsh9]

Hi @s-weigand , if you'd want to make a code change, you are welcome to do so. Thanks!

I think the scope of this proposed config option is limited enough that it won't affect other existing functionalities. So if it benefits some users, let's add it. (And the default value of this option can be "off" to maintain backward compatibility.)

But just note that if we do add this option, we'd need to update the documentation to explain how to write the "raises" section if turn this option on -- specifically, how shall we order the exceptions in the docstring: in the order of appearance in the function/method, or in alphabetical order?

[s-weigand]

My pragmatic solution would be to use alphabetical order on both sides and ignore the order.
While there is a point to make for ordering exceptions in the docstrings in the order of occurrence for easier lookup, I would argue that from a documentation point of view, it makes more sense to group them by type, since the same type conveys the information that the reason for the exception is similar.
At least when I look up exceptions in the documentation or docstring it is for the purpose of handling them, which makes a type grouping most convenient to read.

One way around this design decision would be to give the user the ability to configure and enforce the style they prefer.

--suppress-exception-deduplication
   "off" | "unordered" | "occurrence" | "alphabetical" (default "off")