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

Fix crash when using f-strings #653

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

Fix crash when using f-strings #653

wants to merge 3 commits into from

Conversation

kasium
Copy link
Contributor

@kasium kasium commented Sep 13, 2023 •
edited

Closes #640

Thanks for submitting a PR!

Please make sure to check for the following items:

  • Add unit tests and integration tests where applicable.
    If you've added an error code or changed an error code behavior,
    you should probably add or change a test case file under tests/test_cases/ and add
    it to the list under tests/test_definitions.py.
    If you've added or changed a command line option,
    you should probably add or change a test in tests/test_integration.py.
  • Add a line to the release notes (docs/release_notes.rst) under "Current Development Version".
    Make sure to include the PR number after you open and get one.

Please don't get discouraged as it may take a while to get a review.

@sigmavirus24
Copy link
Member

This prevents the crash but it misleads people and will lead to big reports of "but there's a docstring there" because it doesn't inform people that the f string is invalid and cannot be a docstring. I would not accept this PR as is

@kasium
Copy link
Contributor Author

kasium commented Sep 13, 2023 •
edited

@sigmavirus24 Reason for this is that in #381 the following was suggested

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.

I'm fine adding a new error code saying that an f-string is invalid, but this idea was dropped before

@sigmavirus24
Copy link
Member

The idea for adding a new error code was not dropped. The PR lost steam due to people being busy. The new error code is correct behavior. How we achieve that is up for debate

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash with simple f-string
2 participants
@kasium @sigmavirus24