Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check-docstring-first false positive on attribute docstrings #159

Closed
jaraco opened this issue Dec 23, 2016 · 10 comments
Closed

check-docstring-first false positive on attribute docstrings #159

jaraco opened this issue Dec 23, 2016 · 10 comments

Comments

@jaraco
Copy link

jaraco commented Dec 23, 2016

Attribute docstrings, as defined in PEP-257 are strings immediately following top level module names. For example:

name = "my variable"
"""the name used by the module for x, y, and z"""

The check-docstring-first incorrectly identifies this attribute docstring as a module docstring, indicating either Multiple module docstrings (first docstring on line N). or Module docstring appears after code (code seen on line N)..

In light of this issue, I'm not sure the check-docstring-first check has much value at all. At the very least, the check should not be failing when a docstring follows an assignment.

@asottile
Copy link
Member

The goal of the hook was mostly to prevent imports-then-docstring (an especially common/easy mistake with future imports and docstrings), perhaps it can be adjusted to stop searching after it finds some non-import code (similar to the algorithm used in https://github.com/asottile/reorder_python_imports)

@sdouche
Copy link

sdouche commented Jul 10, 2019

Hey @asottile :)
Do you think is possible to resolve this issue?

@asottile
Copy link
Member

Doubt it, unless you can come up with a good heuristic for differentiating the mistake and the intentional "attribute docstring"

@sdouche
Copy link

sdouche commented Jul 12, 2019

@asottile Understood. Maybe close this issue then?

@asottile
Copy link
Member

It may be possible, I haven't invested any time into this particular issue as I don't use "attribute docstrings" - - normal comments work fine for me

@lukelbd
Copy link

lukelbd commented Nov 29, 2019

@asottile People use "attribute docstrings" becuase the sphinx autosummary documentation generator looks for docstrings following class-level and module-level variables and uses them to generate website documentation. This would actually be a useful feature for a lot of people.

@asottile
Copy link
Member

This issue is still open, if you can come up with a heuristic that properly identifies "attribute docstrings" vs the mistakes it currently catches then I'd happily entertain a tested patch. You'll notice I marked this as a bug and I haven't closed it, so please be careful of asserting that I'm being dismissive. I simply don't have any reason to work on this myself since I don't use that feature, but I'd be happy to guide you through implementing this :)

I'm pretty sure you can also use this as a workaround (or at least I've seen comments like this in the past):

attr = 1
#: comment about the attribute

(I might be misremembering the exact format, it's been a while!)

@lukelbd
Copy link

lukelbd commented Nov 29, 2019

Sorry didn't mean to be rude; I corrected my tone! Obviously I'm super grateful for this project. I didn't know about #: -- maybe that ought to close this issue actually.

@asottile asottile closed this as completed Mar 5, 2021
@jaraco jaraco changed the title check-docstring-first fails positive on attribute docstrings check-docstring-first false positive on attribute docstrings Mar 6, 2021
@ryanrhee
Copy link

ryanrhee commented Jan 18, 2022

I just ran into this. FWIW, #: is not a part of PEP-257, and (more importantly for me,) comments starting with #: are not picked up by vscode the way that attribute docstrings are.

just fyi, that there are use cases for which the #: workaround doesn't work.

JonasPammer added a commit to JonasPammer/ansible-roles that referenced this issue Jul 2, 2022
rexut added a commit to tiacsys/verylittlewire that referenced this issue Nov 17, 2022
Remove check-docstring-first pre-commit hook as it doesn't support
using Sphinx attribute docstrings. Attribute docstrings, as defined
in PEP-257 are strings immediately following top level module names.

The check-docstring-first hook incorrectly identifies this attribute
docstring as a module docstring, indicating either:

- "Multiple module docstrings (first docstring on line N)", or
- "Module docstring appears after code (code seen on line N)"

For details see related discussion on pre-commit issue:

    pre-commit/pre-commit-hooks#159

Signed-off-by: Stephan Linz <[email protected]>
rexut added a commit to tiacsys/verylittlewire that referenced this issue Nov 21, 2022
Remove check-docstring-first pre-commit hook as it doesn't support
using Sphinx attribute docstrings. Attribute docstrings, as defined
in PEP-257 are strings immediately following top level module names.

The check-docstring-first hook incorrectly identifies this attribute
docstring as a module docstring, indicating either:

- "Multiple module docstrings (first docstring on line N)", or
- "Module docstring appears after code (code seen on line N)"

For details see related discussion on pre-commit issue:

    pre-commit/pre-commit-hooks#159

Signed-off-by: Stephan Linz <[email protected]>
rexut added a commit to tiacsys/verylittlewire that referenced this issue Nov 25, 2022
Remove check-docstring-first pre-commit hook as it doesn't support
using Sphinx attribute docstrings. Attribute docstrings, as defined
in PEP-257 are strings immediately following top level module names.

The check-docstring-first hook incorrectly identifies this attribute
docstring as a module docstring, indicating either:

- "Multiple module docstrings (first docstring on line N)", or
- "Module docstring appears after code (code seen on line N)"

For details see related discussion on pre-commit issue:

    pre-commit/pre-commit-hooks#159

Signed-off-by: Stephan Linz <[email protected]>
rexut added a commit to tiacsys/verylittlewire that referenced this issue Nov 25, 2022
Remove check-docstring-first pre-commit hook as it doesn't support
using Sphinx attribute docstrings. Attribute docstrings, as defined
in PEP-257 are strings immediately following top level module names.

The check-docstring-first hook incorrectly identifies this attribute
docstring as a module docstring, indicating either:

- "Multiple module docstrings (first docstring on line N)", or
- "Module docstring appears after code (code seen on line N)"

For details see related discussion on pre-commit issue:

    pre-commit/pre-commit-hooks#159

Signed-off-by: Stephan Linz <[email protected]>
Julian added a commit to DanielNoord/jsonschema that referenced this issue Dec 2, 2022
check-docstring-first has to go unfortunately due to
pre-commit/pre-commit-hooks#159.
@SirTakobi

This comment was marked as off-topic.

@pre-commit pre-commit locked as off-topic and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
6 participants