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

Adds reserved word check to signature generation logic. #4012

Merged

Conversation

strue36
Copy link
Contributor

@strue36 strue36 commented Apr 25, 2022

Change Summary

Adds a check within utils.generate_model_signature() to treat reserved words as invalid identifiers.

Related issue number

fix #4011"

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@Zac-HD
Copy link
Contributor

Zac-HD commented Apr 29, 2022

I found this issue and PR via some reports about Hypothesis, and this is exactly how I'd fix it 👍

(plus the traditional better-error-message in Hypothesis, of course 😅)

@strue36 strue36 marked this pull request as ready for review April 29, 2022 16:33
@strue36
Copy link
Contributor Author

strue36 commented Apr 29, 2022

please review

@Zac-HD
Copy link
Contributor

Zac-HD commented May 11, 2022

@samuelcolvin since my PR was accepted by CPython, this is now a bugfix for Python 3.11.0b1 and later; without it you'll get unhandled ValueErrors.

@samuelcolvin
Copy link
Member

Congrats, I'll merge this once I get 1.9.1 out.

@agronholm
Copy link

Congrats, I'll merge this once I get 1.9.1 out.

1.9.1 is out, so would you mind merging this?

@@ -192,6 +194,15 @@ def in_ipython() -> bool:
return True


def is_valid_identifier(identifier: str) -> bool:
"""
Checks that a string is a valid identifier and not a reserved word.
Copy link
Member

Choose a reason for hiding this comment

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

Based on official iskeyword doc I would

Suggested change
Checks that a string is a valid identifier and not a reserved word.
Checks that a string is a valid identifier and not a Python keyword.

@hramezani
Copy link
Member

Thanks @strue36 for the patch 👍

I just left a small comment regarding the function docstring.

@hramezani
Copy link
Member

please update

@strue36 strue36 requested a review from hramezani July 25, 2022 08:39
@strue36
Copy link
Contributor Author

strue36 commented Jul 25, 2022

Let me know if you want me to squash both commits together.

@hramezani
Copy link
Member

Let me know if you want me to squash both commits together.

No, we will squash it when merging.

@samuelcolvin samuelcolvin merged commit 4d87699 into pydantic:master Aug 8, 2022
@samuelcolvin
Copy link
Member

thanks so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model signatures contain reserved words.
6 participants