bad-docstring-quotes false positive when using decorators

[vapier]

Steps to reproduce

Note: The original code was mocking a module/function path that was longer than 80 cols which is why we had to line wrap it.

Example failing code:

from unittest import mock

@mock.patch('mock.'
            'foo')
class Foo:
    """Blah"""

Running as:

$ pylint --load-plugins=pylint.extensions.docstyle test.py

Current behavior

A false positive is emitted:

test.py:3:0: C0198: Bad docstring quotes in class, expected """, given ' (bad-docstring-quotes)

Expected behavior

No warnings should be emitted.

pylint --version output

pylint 2.3.1
astroid 1.6.0
Python 3.6.4 (default, Jan 28 2018, 15:24:24) 
[GCC 6.4.0]

[PCManticore]

Thank you, I can also reproduce the issue.

[vapier]

looks like @unittest.skipIf decorators also trip this :/

@unittest.skipIf(SomeCondition(),
                 'Tests only make sense when some condition is True')

C: 23, 0: Bad docstring quotes in class, expected """, given ' (bad-docstring-quotes)

[kasium]

Hi all, so I also found this issues. It was reported here to python and was fixed by python/cpython#9731. However, this change didn't make it to python 3.7.

In the python bug report they reference PyCQA/pyflakes#273 which includes a handy workaround. Should that maybe be used in pylint?

@Pierre-Sassoulas what do you think. I'm willing to open a PR. Maybe astroid is the better place?

[Pierre-Sassoulas]

Thank you for bringing that up @kasium, it seems like it could be useful for a long time if it affects python 3.6 and 3.7, I would definitely merge something that fix this in pylint. It seems like it's an ast issue so it's probably in astroid like you said, but creating a functional test in pylint is always a nice start to be able to check if you're local version of astroid fix the issue. (See #5269 for an example of cross-issue between pylint/astroid)

[kasium]

So, I checked this further and the workaround noted above are using indicators like a base class or function parameters. However, if they are missing, they don't work. Also, in astroid, the docstring is not a node so it's line number cannot be received.

I would recommend, to add a new doc_node attribute to nodes. It's the string as a node. This should definitively help. @Pierre-Sassoulas what do you think?

[Pierre-Sassoulas]

This sounds like a good idea but I don't know how hard adding this doc node attribute would be. If the ast module does it, we need to handle multiple version of the python interpreter, if it does not it could be tricky.

[kasium]

@Pierre-Sassoulas I think it's possible. So for instance, the ClassDef is built here [1] and it's using this [2] little helper function to get the docstring. Then the docstring is removed from the nodes body.
So, this means, that I can just add a new optional argument to the ClassDef constructor which is the original docstring node. This can be done quite similar for FunctionDef I guess. Since everything is optional, it should not break anything. Then in pylint we can just check, if the node has a doc_node attribute. If yes, new logic, else old one.

Any feedback?

[1] https://github.com/PyCQA/astroid/blob/3a1cdb0d0daf959537a15b547dffdf9ae9dc3dc9/astroid/rebuilder.py#L1174
[2] https://github.com/PyCQA/astroid/blob/3a1cdb0d0daf959537a15b547dffdf9ae9dc3dc9/astroid/rebuilder.py#L107

[Pierre-Sassoulas]

It sounds like a great enhancement to astroid without any disadvantages :)

[kasium]

@Pierre-Sassoulas coming back from the astroid discussion, what about disabling the check for <= py3.7?

[Pierre-Sassoulas]

what about disabling the check for <= py3.7?

Sounds good !

[kasium]

Okay, let me prepare a PR in the next days