Docstr comment

[gschaffner]

Steps

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Description

Fix spell-checker crash on indented docstring lines that look like # comments. See #3786 for more info.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes #3786

[gschaffner]

Should a new test be added for this as well?

[coveralls]

Coverage remained the same at 90.721% when pulling 8e7e8f7 on gschaffner:docstr_comment into 5728d82 on PyCQA:master.

[Pierre-Sassoulas]

Hello, thank you for the PR. The examples you gave in #3786 seems good candidates for a functional test, you can add them in tests/functional/. Also, I don't know what kind of work would be involved or if I understood what was done correctly, but would it be possible to fix the "docstring" msgid treatment instead of skipping it?

[gschaffner]

Hi, thanks for the reply! I do not understand tests/functional very well, and I cannot seem to find any functional tests of the spell checker. Would https://github.com/PyCQA/pylint/blob/3e7e3ca443b9355cbc13467d9c3b8fa794647410/tests/checkers/unittest_spelling.py be a better place to add such tests? Something like https://github.com/PyCQA/pylint/blob/3e7e3ca443b9355cbc13467d9c3b8fa794647410/tests/checkers/unittest_spelling.py#L175-L191

Also, I may be misunderstanding you but I don't think that my change skips any treatment. A line of a docstring that looks like a comment should not have its leading # trimmed off as if it was a comment. To the interpreter, such a line isn't a comment—it's part of a string.

[Pierre-Sassoulas]

No problem ! Yes adding unit test would be a plus too :)

For functional tests, you can add your example like what was done in this merge request in tests/functional/l/line_too_long.py and tests/functional/l/line_too_long.txt . You can create both files with the name you want or use another pre-existing file, it have to be inside "tests/functional/" and have a .py and a .txt with the same basename.

[gschaffner]

It seems to me that this needs to be covered by unit tests rather than functional tests. I believe there aren't currently any functional tests of the spell checker because (AFAIK)

• There is not a way to set --spelling-dict en_US for functional tests. Pylint won't report spelling issues if spelling-dict is not set.
• There is not a way to handle multiline pylint errors (like wrong-spelling-in-docstring) in tests/functional/f/foo.txt.

[Pierre-Sassoulas]

Ho, ok, I did not use spell checking much. It makes sense to just use unit test then.

[Pierre-Sassoulas]

This seems ready for merging, thank you. @gschaffner, could you rebase on the latest master to fix the conflicts, please ?

[gschaffner]

Done! @Pierre-Sassoulas

Edit: Don't merge yet, I realized that we should use visit_functiondef instead of visit_classdef. One second. Done.