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

Allow single generic param for Field in ForeignKey #2261

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

rafonseca
Copy link
Contributor

@rafonseca rafonseca commented Jul 15, 2024

Allow a less verbose specification of foreign keys

This set he default type of getter so that the user can choose to specify the foreign key using one or two generic params.

Recently, I've moved from django-types to django-stubs, and this modification allowed for a smooth transition.

Related issues

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

This looks fine to me but a bunch of plugin code is about/around these _ST and _GT type variables so for that reason and any future changes, I would like that a test is included here that uses a single generic argument. Would be great if both OneToOneField and ForeignKey are included in that test

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Yes, please, add a test for single-arg Field

@rafonseca
Copy link
Contributor Author

I've forgot to mention I used this with pyright... I'm not acquainted with mypy but what I understand here is that either mypy does not propagates the type defined in the first parameter to the second parameter or it is a printing issue. In the latter case, maybe there is another way to write the test.
@sobolevn , @flaeppe , can you give me some advice here?

@flaeppe
Copy link
Member

flaeppe commented Jul 16, 2024

I think it's the former, mypy not handling type vars as default, while recognizing what you say from python/mypy#14851 (comment)

Our own related issue is #1264

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

Since Mypy doesn't seem to break by adding the default= argument I would be fine adding it already for the sakes of pyright.

Which hopefully allows us to just fill in for Mypy later on when python/mypy#14851 (comment) is resolved

We could add expect_fail: true (xfail) for the 2 new tests to start with

tests/typecheck/models/test_related_fields.yml Outdated Show resolved Hide resolved
tests/typecheck/models/test_related_fields.yml Outdated Show resolved Hide resolved
@rafonseca
Copy link
Contributor Author

rafonseca commented Jul 16, 2024

Thanks for the quick answer @flaeppe . I've updated with your suggestions. Should we add any note about the failing test or the link to this PR in the final commit message is enough?

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

No need to add a note, I feel like the git history and link to this PR will be enough.

@sobolevn sobolevn merged commit b2b1afa into typeddjango:master Jul 16, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants