terrencepreilly / darglint

A python documentation linter which checks that the docstring description matches the definition.
MIT License
481 stars 41 forks source link

False Positive for DAR103 with optional arguments #123

Closed diabolical-ninja closed 4 years ago

diabolical-ninja commented 4 years ago

It appears that optional arguments are generating a false positive alert on DAR103. For example, this function:

def a_fn(x: str = "default"):
    """Summary.

    Args:
        x (str, optional): Val. Defaults to "default".
    """
    pass

generates:

>> darglint function_file.py
function_file.py:a_fn:5: DAR103:  ~x: expected str but was str, optional

I think this is a repeat of #1 but as that issue was closed, I've created a new one.

terrencepreilly commented 4 years ago

Darglint no longer supports that type format. See issues #114, #112, and #95. The main part of the problem is that each docstring style (Google, Sphinx, Numpy, etc.) introduces different styles for these types. And none of the styles are actually capable of correctly describing all types. (Consider a list of optional strings. With type hints, it would be List[Optional[str]]. What would it be in this format? list, str, optional? list str-optional?)

Instead, Darglint accepts the same type signature accepted by Python type hints. So instead of (str, optional), you could put (Optional[str]). Of course, it will raise an error, because your type signature doesn't match. (Your function doesn't actually accept None as an option.)

We could probably parse these informal type descriptions, but it would be a good deal of work without much benefit. (Since type hints have been introduced.) If you'd like to take a crack at it, feel free to open a pull request. As it stands, it's pretty low priority for me.

diabolical-ninja commented 4 years ago

Thanks for the explanation @terrencepreilly, that makes sense & is much more complex than it seems on the surface.

taranlu-houzz commented 3 years ago

@terrencepreilly I am running across this issue now and am wondering if there are any other solutions? I understand why darglint does not support the optional/default format unless you use typing.Optional but it seems like this usage goes directly against what the Python documentation says:

Note that this is not the same concept as an optional argument, which is one that has a default. An optional argument with a default does not require the Optional qualifier on its type annotation just because it is optional. For example:

It seems to me that if one never intends to allow for None to be passed, then using typing.Optional isn't really correct.

terrencepreilly commented 3 years ago

The selection you quoted isn't saying that optional isn't required if there is a default value. It's saying that if a default value is given, we don't have to use optional type because an unspecified value takes the value given as the default.

Sorry if that's confusing. I'm not sure that I'm explaining it in an understandable fashion. Maybe giving an example will clarify things? Say we have a function,

def my_function(x:int = 5):
    print(x)

The documentation you quoted is saying that we don't have to type the argument x as Optional[int], even though it's not a required field. (That is, it is optional whether we specify the value passed to my_function.). If we pass no value in for x, it will take the value 5, otherwise, it will take the value passed.

It's pretty confusing. In any case the above type signature is correct. It's communicating that we don't expect the value None to be passed to the function. If we call my_function(None), it will print "None", not "5", because a value was passed. But we will get a type error, even through it allows it at runtime. Feel free to ask follow up questions -- it's legitimately confusing.

I closed this issue not because I'm doing any type checking in Darglint. Rather, I'm just deferring to the default Python type hints. It makes it easier for me, and avoids a lot of ambiguity present in some of the type comments present in documentation formats before type hints were introduced. As usual, gradual typing leads to a lot more confusion, even while reducing ambiguity (kind of a conundrum.)

taranlu-houzz commented 3 years ago

@terrencepreilly Ah, sorry, I think maybe I didn't fully explain the issue. It's possible I am doing something wrong, but what I am running into is that the following generates an error with darglint and the only solution that seems to fix the error is to use Optional but that has a very specific meaning that would be wrong in this case:

def my_func(
    arg1: int = 60,
) -> None:
    """Test darglint handling param with default.

    Parameters
    ----------
    arg1 : int, default=60
        Test default param.
    """

    pass
DAR103 Parameter type mismatch:  ~arg1: expected int but was int, default=60

The only solution that I have found is to use arg1: Optional[int] = 60 in the function definition, which gets rid of the darglint error, but then it implies that None is a valid/expected argument, which is not the case.

Edit: I suppose another option would be to remove the default=60 in the Parameters listing, but then that goes against the numpy doc spec (I think?).

hectormz commented 3 years ago

Is it possible to make another DAR### error, separate from DAR103 that only includes expected XX but was XX, optional, so I can ignore that code but not miss any mismatches in DAR103?

It's a level of messiness in darglint, but would let me keep typehints that make sense to me

MartinBernstorff commented 2 years ago

I would strongly, strongly endorse @hectormz point here. Love darglint, but 85% of the errors are false positives with the "optional" flag.

waltergallegog commented 2 years ago

Running into the same issue here. For now I'm filtering out the lines of the form expected XX but was XX, optional using grep and regex. I use flake8 so I have the alias :

alias flake='flake8 | grep -v -E "expected (\w+) but was \1, optional"'