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

Fix: FindInterface type-hints break on View models #819

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

GuyGooL5
Copy link
Contributor

@GuyGooL5 GuyGooL5 commented Jan 1, 2024

This PR adds a new TypeVar for View just like DocType, and is used in FindInterface to bind it for each model with their respective model type.

It fixes a bug where find operations type-checks break when used with View descendants. For example, see this issue.

@GuyGooL5 GuyGooL5 changed the title fix: FindInterface type-hints break on View models Fix: FindInterface type-hints break on View models Jan 1, 2024
@roman-right
Copy link
Member

Hi @GuyGooL5 ,
Could you please run pre-commit against your changes?
Thank you for the PR!

@GuyGooL5
Copy link
Contributor Author

Hi @roman-right, unfortunately I've found that my solution isn't all-inclusive and there are many edge cases, I'm drafting this PR and will work on it.

@GuyGooL5 GuyGooL5 marked this pull request as draft January 11, 2024 10:21
@roman-right
Copy link
Member

Hi @GuyGooL5 ,
Yes sure. Thank you for your work!

@@ -1012,7 +1012,8 @@ def get_previous_changes(self) -> Dict[str, Any]:
return {}

return self._collect_updates(
self._previous_saved_state, self._saved_state # type: ignore
self._previous_saved_state,
self._saved_state, # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this has nothing to do with the PR, it was simply auto-formatted.

@GuyGooL5 GuyGooL5 marked this pull request as ready for review January 13, 2024 12:57
@GuyGooL5
Copy link
Contributor Author

@roman-right I hope that's enough for a fix. I tested the type inference in VSCode and it seems to work, is there maybe a test I can implement to future-proof it from a static-type checker's perspective?

@roman-right
Copy link
Member

Hi @GuyGooL5 ,
Thank you for the PR! I'll check it this week

@roman-right roman-right merged commit 63542eb into BeanieODM:main Jan 22, 2024
21 checks passed
@roman-right
Copy link
Member

Hi @GuyGooL5 ,
Thank you for your work. Merged. It will be published in a few days

@GuyGooL5 GuyGooL5 deleted the fix/find-interface-for-views branch January 24, 2024 13:48
@GuyGooL5
Copy link
Contributor Author

Great news, thanks.

@roman-right roman-right mentioned this pull request Jan 25, 2024
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.

2 participants