ci: Add pydoclint to pre-commit hooks

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (74996cd) 100.00% compared to head (2a107cb) 100.00%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #512 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 53 53 Lines 2388 2388 Branches 599 599 ========================================= Hits 2388 2388 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[SiQube]

I'm extremely conflicted about this PR. it is a lot of overhead but would improve readability for sure.

[SiQube]

I'll delete all the default values because it is not checked by the linter and hence manual effort to control for future PRs, opinions? @prassepaul @dkrako

[dkrako]

I'll delete all the default values because it is not checked by the linter and hence manual effort to control for future PRs, opinions? @prassepaul @dkrako

I agree. As we don't lint for default values (yet?), you can delete all default values from the docstrings.

[dkrako]

Are there any updates for this PR?

If removing the Attributes and Raises sections was due to some pydoclint error, I would very much prefer to put an ignore to the specific error code and fix the problems in separate PRs. Otherwise the scope of this PR will be too big to handle.

[SiQube]

ah sorry forgot -- yes the raises and so on were from pydoclint, I'll look for an argument to ignore this.

[dkrako]

Due to no identified error code I am afraid that there is no way to just ignore errors with the Raise section.

If there's no other way for ignoring the error we can do a workaround then:

try:
   check_something(variable)
except ValueError as e:
    raise e

and actually, if we think that through, pydoclint is technically in the right: if we look at the stack-trace, the exception is not thrown by the documented outer function, but by the inner check function. otherwise we would have to specify a lot of useless exceptions (e.g. basically every function can be aborted by KeyboardException) and we would need to include that in every function.

In the next step we have to find out why the attributes section is failing. Do you have the particular error message from pydoclint?

[SiQube]

and actually, if we think that through, pydoclint is technically in the right: if we look at the stack-trace, the exception is not thrown by the documented outer function, but by the inner check function. otherwise we would have to specify a lot of useless exceptions (e.g. basically every function can be aborted by KeyboardException) and we would need to include that in every function.

that's why I deleted them from the documentation -- what is your opinion, should I readd them?

In the next step we have to find out why the attributes section is failing. Do you have the particular error message from pydoclint?

I'll look into this when we decided to add pydoclint -- it is a lot of work especially with the big refactors (see conflicts above) but I'd be willing to do it :)

[dkrako]

and actually, if we think that through, pydoclint is technically in the right: if we look at the stack-trace, the exception is not thrown by the documented outer function, but by the inner check function. otherwise we would have to specify a lot of useless exceptions (e.g. basically every function can be aborted by KeyboardException) and we would need to include that in every function.

that's why I deleted them from the documentation -- what is your opinion, should I readd them?

I think just completely removing this from our documentation is not that good. Did you find a way to ingore this? The same also with the attributes.

In the next step we have to find out why the attributes section is failing. Do you have the particular error message from pydoclint?

I'll look into this when we decided to add pydoclint -- it is a lot of work especially with the big refactors (see conflicts above) but I'd be willing to do it :)

I don't understand. Should we reevaluate if we will add this? I think it's totally worth it.

But I would advise to put errors that need a lot of work into the list of ignores. We can then tackle them one by one in separate PRs.

Focus on finishing this PR without doing much extra work. Otherwise you will just get new merge conflicts next time.

[dkrako]

Regarding the errors with the raises:

Can you list the viable options that we have? I don't remember anymore if the raise errors can be ignored?

[SiQube]

problem is:

def a():
    """
    Function just raises stuff.

    Raises
    ------
    Exception
        foo bar
    """
    raise Exception

def b():
    """
    Function does nothing.
    """
    a()

def c():
    """
    Function does nothing.

    Raises
    ------
    Exception
       foo bar
    """
    a()

def d():
    """
    Function just raises stuff.
    """
    raise Exception

the problem is now that we get an error, for function c we get DOC502: Function c has a "Raises" section in the docstring, but there are not "raise" statements in the body

for function b we lose the documentation of the exception

there are a few different options

1. ignore all "errors" with raises pro: no problems if a raise can happen in an inhereting way (no error thrown for function c) con: no error when the unit has no docstring documentation for the raise (no error thrown for function d)

2. try except approach as discussed here

try:
   check_something(variable)
except ValueError as e:
    raise e

I can't think of any other option

[dkrako]

Thank you for the update. Let's keep it simple first with as little code changes as possible, so let's ignore DOC502 for this PR.

We can then create an issue for resolving the raise problems, as we need to have a more thorough discussion if our exception raising needs an overhaul.

[SiQube]

closed in favor of #638