typing: corrections for dbcore/types.py #4828
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wisp3rwind
force-pushed
the
dbcore_typing_2
branch
2 times, most recently
from
813cdac
to
f98987c
Compare
|
Oh, given the second commit here: Shall we drop Python 3.7 very soon? It's EOL in a few days if I'm no mistaken. |
sampsyo
reviewed
Jun 24, 2023
There was a problem hiding this comment.
Wow, this is very tricky indeed! It was a really good idea to parameterize Type on both a "main" type and a null type; this seems to help clarify types work in general (i.e., on "input," you're supposed to provide the main type; on "output," you need to expect that you might get the null type as well). Really nice work nailing this down!
I have few assorted thoughts scattered throughout.
wisp3rwind
force-pushed
the
dbcore_typing_2
branch
from
f98987c
to
6314e9f
Compare
|
Thanks for the swift and thorough review! I've addressed the comments and rebased, let's see what CI says. |
wisp3rwind
force-pushed
the
dbcore_typing_2
branch
from
6314e9f
to
a1c4a53
Compare
tricky... - the only way I found to express the concept of the "associated type" (in Rust lingo) model_type was by making Type generic over its value and null types. - in addition, the class hierarchy of Integer and Float types had to be modified, since previously some of them would have conflicting null types relative to their super class (this required a change to the edit plugin; hopefully no more breakage is caused by these changes) - don't import the query module, but only the relevant Query's to avoid confusing the module query and the class variable query
wisp3rwind
force-pushed
the
dbcore_typing_2
branch
from
a1c4a53
to
e9f3565
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Yet another follow-up to dbcore typing. I've had this in the pipeline for a while, but only now got around to finishing it up and figure out the last bits around handling the
model_typeType-valued field.Please have a close look at this PR; does the approach look sensible? I restructured the class hierarchy a bit to avoid some conflicting signature. This could potentially break plugins that depend on details of the db and type handling (as it turns out, the edit plugin does).