Allow ignoring errors using code comments

[llucax]

Like most linters, it would be very useful being able to ignore a certain type of error for a particular function/class/module.

With documentation linting is harder to achieve, as annotating docstrings

Darglint allowed this but by annotating the docstrings, but that is not a good option, as the annotations will show up then in the help, which is unhelpful and confusing to users.

The flexibility of the scheme # noqa: <error> <argument> is very useful though. For example, to only ignore the checking of one argument, one can do:

def a_bound_function(self, arg1):
  """Do something interesting.

  Args:
    arg1: The first argument.

  # noqa: DAR101 arg1
  """

Or to ignore only one type of exception in Raises:

def always_raises_exception(x):
    """Raise a zero division error or type error.o

    Args:
      x: The argument which could be a number or could not be.

    Raises:
      ZeroDivisionError: If x is a number.  # noqa: DAR402
      TypeError: If x is not a number.  # noqa: DAR402

    """
    x / 0

I suggest, if possible, adding the # noqa: comments directly in the code, for the above examples:

def a_bound_function(self,
    arg1 # noqa: DAR101
):
  """Do something interesting.

  Args:
    arg1: The first argument.
  """

def always_raises_exception(x):  # noqa: DAR402 ZeroDivisionError TypeError
    """Raise a zero division error or type error.o

    Args:
      x: The argument which could be a number or could not be.

    Raises:
      ZeroDivisionError: If x is a number.
      TypeError: If x is not a number.

    """
    x / 0

Using the equivalent codes from pydoclint, of course.

[llucax]

Here is an use case: https://github.com/frequenz-floss/frequenz-channels-python/blob/922c7c33a5bebd30d3b0b8196749270773461a65/src/frequenz/channels/_base_classes.py#L190-L199

We want to have the exception specified because it is following an interface (and the code can actually raise that exception, just indirectly).

Here is the error I get:

src/frequenz/channels/_base_classes.py
    190: DOC502: Method `_Map.consume` has a "Raises" section in the docstring, but there are not "raise" statements in the body 

[jsh9]

Hi @llucax ,

pydoclint can do function-level violation omission in the flake8 mode.

For example,

def myFunc(  # noqa: DOC101, DOC103
        arg1: int,
        arg2: str,
) -> None:
    """
    Do something

    Parameters
    ----------
    arg1 : float
        Arg 1
    """
    pass

The # noqa comment needs to be put on the line where the function name (myFunc) is.

But # noqa only takes effect if you run it via flake8 myFile.py, not when you run it via pydoclint myFile.py.

I mentioned in this part of README that "noqa" omission in the native mode may be added if there are demands for it.

---

That said, I don't think per-argument omission is feasible in pydoclint's current architecture. This is because when pydoclint uses ast to parse a function, the whole function signature (such as def funcName(arg1: int, arg2: str, ...) -> None) is parsed together, regardless of any line breaks.

I also checked the parsed node, and I could not find any trace of # noqa comments when they are put after a particular argument.

If we want to parse per-argument # noqa, we might need to write a custom code parser. And as a result, we may end up with as slow a speed as Darglint?

[llucax]

Hi @jsh9,

I see, thanks for the detailed explanation and sorry for not noticing the flake8 mode, we are not using flake8 for the moment and never dived into the flake8 world yet, but maybe I'll do it in the future. I think it would also be very helpful to include an example about how to add # noqa, as there is no explicit mention to it, nor to the limitations on where should it be put.

I think astroid, the parser used by pyint, can also gather the information at this level, but pylint is also very slow, not sure if it's because of the parser or something else.

Is there any way to add "arguments" to the noqa in flake8 mode, so at least we could select which exceptions to ignore? As I don't really want to stop checking for all exceptions, only a few ones that are raised by functions called inside the body.

As a very hacky solution I could also do:

def always_raises_exception(x):
    """Raise a zero division error or type error.o

    Args:
      x: The argument which could be a number or could not be.

    Raises:
      ZeroDivisionError: If x is a number.
      TypeError: If x is not a number.
    """
    x / 0
    # pylint: disable=unreachable
    raise ZeroDivisionError()  # type:ignore
    raise TypeError()  # type: ignore

But it is quite nasty. I think being able to ignore certain type of exceptions in the check is super important, because as shown in the code above, even python operators can raise exceptions, what would be the alternative, wrap the division in a try/except block and create a new ZeroDivisionError()?

If I do this:

    try:
        x / 0
    except ZeroDivisionError:
        raise

Then pylint complains about having an except block that just re-raises, which is completely unnecessary, so it is a valid complain :)

Then I have to do this, which is even more verbose:

    try:
        x / 0
    except ZeroDivisionError as exc:
        raise ZeroDivisionError("Cannot divide by zero.") from exc

And also adds unnecessary runtime costs, as I'm creating new objects, and makes the stack trace harder to read, as I have 2 ZeroDivisionErrors for no reason.

I don't think this feature is complete without being able to ignore specific exceptions.

[jsh9]

If you'd like to ignore DOC104 and DOC201 in a function, you can simply do this:

def myFunc(...):  # noqa: DOC104, DOC201
    ....

Is this the granularity that you are looking for?

[jsh9]

Also, I added a section in the documentation on how to ignore certain violation codes: 8e0ccce

[llucax]

Is this the granularity that you are looking for?

No, I meant being able to ignore certain exception types for DOC502, so being able to say something like:

def raises_exceptions(x): # noqa: DOC502(ZeroDivisionError, TypeError), DOC501(AssertError)
    """Raise a zero division error or type error.o

    Args:
      x: The argument which could be a number or could not be.

    Raises:
      ZeroDivisionError: If x is a number.
      TypeError: If x is not a number.
      WeirdException: blah.
    """
    if x:
        x / 0
    elif x > 0:
        raise MyException()
    else:
        raise AssertError()

So this should complain about MyException not being documented, about WeirdException not being raised and nothing else (so ZeroDivisionError, TypeError and AssertError should not trigger an error because they are declared to be ignored).

I guess the broad disabling of the whole check is not that bad, but it would be very nice to still catch mistakes even if there are some exceptions that are raised indirectly.

[llucax]

Also, I added a section in the documentation on how to ignore certain violation codes: 8e0ccce

Thanks!

[jsh9]

Hi @llucax , what you are looking for (# noqa: DOC502(ZeroDivisionError, TypeError), DOC501(AssertError)) may be doable, but it might really slow down pydoclint significantly.

I'll label this as "may tackle in the future" and close this issue for now.

But if you have any additional questions or comments, please feel free to add them below. Of if you'd like to re-open it for another reason, please feel free to do so.

Thanks!

[llucax]

Fair enough.

[llucax]

Hi @jsh9, sorry to comment on this closed issue, but maybe this info will change the perception about it and maybe consider reopening.

I just tried using flake8 to run pydoclint so I can use the ignore rules. Running with flake8 is not really Slightly slower as stated in the readme:

Running on https://github.com/frequenz-floss/frequenz-sdk-python/:

$ time flake8 --select=DOC src
...
real    0m10.562s
user    1m1.794s
sys     0m0.530s
$ time pydoclint src
...
real    0m0.189s
user    0m0.156s
sys     0m0.033s

flake8 is 2 orders of magnitude (100x) slower than running directly ❗ 🤯

It is still an improvement over darglint for sure (it takes 24s), but it is on the same order of magnitude as darglint and running directly is still the difference between running instantaneously and having to wait for it, which from the usability POV is huge (for example if it is instantaneous it can be used as a commit hook).

Also maybe you should consider updating the README to be more accurate about the differences between running as a flake8 plugin or not.

[jsh9]

Hi @llucax , I pulled frequenz-sdk-python (the latest on the main branch: frequenz-floss/frequenz-sdk-python@1186f6f) and tested it myself:

flake8 --select=DOC src  1.80s user 0.24s system 462% cpu 0.441 total
pydoclint src  0.11s user 0.02s system 96% cpu 0.132 total

(somehow I didn't get the nice formatting that you had. I'm using macOS with zsh.)

There is still a big difference between running with flake8 and with native pydoclint, but somehow not as drastic as on your computer/environment.

I'm curious what flake8 version do you have on your end? Mine is 6.0.0.

[llucax]

$ flake8 --version
6.1.0 (darglint: 1.8.1, mccabe: 0.7.0, pycodestyle: 2.11.0, pydoclint: 0.2.2, pyflakes: 3.1.0) CPython 3.11.4 on Linux

BTW, flake8 seems to be installed as a dependency from some other package, as we don't use it direclty. Isee there are a lot of other plugins (I assume that's what's being listed), including darglint, maybe that's what's making flake8 slower? Not sure if --select=DOC should inhibit all the other plugins to be loaded or not

[llucax]

OK, I just uninstalled darglint and now I get:

real    0m0.407s
user    0m3.342s
sys     0m0.349s

Twice as slow as running it directly but still sub-second, which is good enough. Thanks and sorry for the noise!

[real-yfprojects]

(somehow I didn't get the nice formatting that you had. I'm using macOS with zsh.)

I guess you are referring to the markdown formatting in his comment.

That can be done by writing

```console
$ command
output
```