Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Add error D303 when an f-string is used as a docstring #381

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

lordmauve
Copy link
Contributor

Emit a new error code D303 when an f-string is found where a docstring is expected.

Fixes #368.

@lordmauve
Copy link
Contributor Author

Tests are green locally under 3.5-3.7 except for the stemming issue that #379 addresses.

sobolevn
sobolevn previously approved these changes Jul 28, 2019
src/pydocstyle/checker.py Outdated Show resolved Hide resolved
@sobolevn
Copy link
Member

Thanks a lot, @lordmauve! I rely on this change in wemake-services/wemake-python-styleguide#579

@sobolevn
Copy link
Member

sobolevn commented Nov 19, 2019

@Nurdok friendly ping. What are the steps to get this merged?

Some of my users rely on this fix: wemake-services/wemake-python-styleguide#579

@lordmauve lordmauve force-pushed the fstrings branch 2 times, most recently from 417d67d to c1d5da2 Compare November 20, 2019 14:30
@lordmauve
Copy link
Contributor Author

Grr, I suck at git, trying to get this rebased and mergable... I don't know where 14a59e6 came from.

@lordmauve
Copy link
Contributor Author

Ok, I think that's clean

@lordmauve
Copy link
Contributor Author

Tests are now failing because mypy returns an error when it sees fstrings under Python 3.5, even though the file won't be imported under Python 3.5, and it appears impossible to tell mypy to ignore this. Looking for a workaround.

@sobolevn
Copy link
Member

# type: ignore to the rescue!

@lordmauve
Copy link
Contributor Author

That does not work; mypy continues to parse the file and raise the error.

@Nurdok
Copy link
Member

Nurdok commented Nov 21, 2019

Sorry for the long delays in response - I've been on back-to-back family vacation and business trips. I'll to catch up as soon as I can.

src/tests/test_cases/test.py Outdated Show resolved Hide resolved
src/pydocstyle/checker.py Outdated Show resolved Hide resolved
src/tests/test_definitions.py Outdated Show resolved Hide resolved
src/tests/test_cases/fstrings.py36 Outdated Show resolved Hide resolved
src/pydocstyle/checker.py Outdated Show resolved Hide resolved
docs/release_notes.rst Outdated Show resolved Hide resolved
@sambhav
Copy link
Member

sambhav commented Mar 7, 2020

@lordmauve any updates on this?

@sambhav
Copy link
Member

sambhav commented Aug 29, 2020

Closing this as it has been inactive for a while. Please feel free to re-open if this is still relevant.

@sambhav sambhav closed this Aug 29, 2020
@sambhav sambhav added the STALE-PR Label for stale PRs which have been closed due to inactivity. label Aug 29, 2020
@sambhav sambhav reopened this Sep 6, 2020
@sambhav sambhav force-pushed the fstrings branch 3 times, most recently from c674d84 to 8a97078 Compare September 6, 2020 16:56
@sambhav sambhav removed the STALE-PR Label for stale PRs which have been closed due to inactivity. label Sep 6, 2020
@sambhav sambhav requested a review from Nurdok September 6, 2020 16:58
sambhav
sambhav previously approved these changes Sep 6, 2020
Copy link
Member

@sambhav sambhav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and I have also addressed all of @Nurdok's comments

@sambhav
Copy link
Member

sambhav commented Sep 6, 2020

@Nurdok please take a look when you have time - I make the code more resilient to fstrings in case people ignore it even after the warnings.

@sambhav
Copy link
Member

sambhav commented Sep 25, 2020

@Nurdok will you have a chance to look at this? It LGTM but waiting on your review.

return sorted(all, key=lambda this_check: not this_check._terminal)

# Note - this needs to be listed before other checks
# as f string evalutaion may cause malformed AST Nodes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - other comments spell this out as "f-string". Please add a hyphen here as well.


f-strings are not treated as string literals, but they look similar
and users may attempt to use them as docstrings. This is an
outright mistake so we issue a specific error code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text will be emitted when running with --explain, so it should explain the problem with the code. The current description seems more "internal" in phrasing. How about this?

"""D303: Docstring may not be f-strings.

f-strings are not treated as string literals by the Python interpreter and are not parsed as docstrings.
"""

(trim to appropriate line length)

# If the docstring is a fstring, it is
# not considered a valid docstring. See
# https://bugs.python.org/issue28739
raise ParseError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistakenly using an f-string for a docstring does not cause a syntax error, so we shouldn't raise a ParseError here.
Instead, I suggest we rename this as eval_docstring and just return an empty string if we find an f-string.

return sorted(all, key=lambda this_check: not this_check._terminal)

# Note - this needs to be listed before other checks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks should not rely on order, other checker functions, etc. Let's find a better solution.
Perhaps my suggestion for returning an empty string from eval_docstring is enough to solve this. Otherwise, we can have it return None and do more verbose checks at call sites. LMKWYT.

@kasium kasium mentioned this pull request Sep 13, 2023
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValueError: malformed node or string
4 participants