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

Ensure the docstring is a string #11143

Merged
merged 7 commits into from
Jul 4, 2023
Merged

Conversation

tushar-deepsource
Copy link
Contributor

@tushar-deepsource tushar-deepsource commented Jun 26, 2023

Resolves #11140

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Thanks, I'm under the impression this regression came out of the new expressions handling and we can simplify this once python 3.7 support is dropped

@The-Compiler
Copy link
Member

Thanks, I'm under the impression this regression came out of the new expressions handling

That part is correct - with 9335a0b, we now do astStr = ast.Constant, so this catches any constant rather than just strings.

and we can simplify this once python 3.7 support is dropped

That part is not, I believe - the only thing that changes is that we don't have ast.Str anymore, only ast.Constant - thus we still need to have this check.

Thanks @tushar-deepsource! ✨

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! It'd be great if you could do those:

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

    Write sentences in the past or present tense, examples:

    • Improved verbose diff output with sequences.
    • Terminal summary statistics now use multiple colors.

    Also make sure to end the sentence with a ..

  • Add yourself to AUTHORS in alphabetical order.

I also think we should backport this to 7.4 once merged.

@bluetech
Copy link
Member

Yep this was definitely an oversight in 9335a0b.

Along with this change we should also audit the other changes in that commit, but I can do it separately.

@Zac-HD Zac-HD dismissed The-Compiler’s stale review July 4, 2023 17:18

Comments addressed

@Zac-HD Zac-HD merged commit 084d756 into pytest-dev:main Jul 4, 2023
@bluetech
Copy link
Member

bluetech commented Jul 5, 2023

Along with this change we should also audit the other changes in that commit, but I can do it separately.

Did this, I think this one was the only problematic case.

bluetech pushed a commit to bluetech/pytest that referenced this pull request Dec 31, 2023
(cherry picked from commit 084d756)

[ran: adapted to 7.4.x, fixed changelog issue number]
bluetech pushed a commit to bluetech/pytest that referenced this pull request Dec 31, 2023
(cherry picked from commit 084d756)

[ran: adapted to 7.4.x, fixed changelog issue number]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite fails when first expression of file is a number and mistaken as docstring
5 participants