E528 fix: update typevar handling when default is not set by pmmmwh · Pull Request #7719 · pydantic/pydantic · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@pmmmwh
Copy link
Contributor
@pmmmwh pmmmwh commented Oct 2, 2023

Change Summary

I believe the behaviour change in #7606 is raising false positives when the default argument is supported but not specified.

Ref: https://peps.python.org/pep-0696/#implementation

Related issue number

#7606

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @Kludex

pmmmwh added 2 commits October 2, 2023 19:36
I believe the behaviour change in #7606 is raising false positives when the `default` argument is supported but not specified.

Ref: https://peps.python.org/pep-0696/#implementation
@pmmmwh
Copy link
Contributor Author
pmmmwh commented Oct 2, 2023

please review

@Kludex Kludex requested a review from adriangb October 3, 2023 06:43
Comment on lines 1454 to 1455
if default is None:
default = not_set
Copy link
Member

Choose a reason for hiding this comment

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

Can the default not be None? How does this differentiate default=None from no default?

Copy link
Contributor Author
@pmmmwh pmmmwh Oct 3, 2023

Choose a reason for hiding this comment

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

According to the PEP:

  • when default is unset, __default__ would be None
  • if default=None, __default__ would be NoneType

Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of our internal unset thing then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yes - let me try and see if that works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@pmmmwh pmmmwh requested a review from adriangb October 5, 2023 09:32
@adriangb adriangb added the relnotes-fix Used for bugfixes. label Oct 5, 2023
@adriangb adriangb merged commit 32ea570 into pydantic:main Oct 5, 2023
@pmmmwh pmmmwh deleted the patch-1 branch October 5, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0