-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Make RowTypeConstraint callable, test nested optional row in schemas and RowCoder #22899
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
Conversation
|
R: @chamikaramj |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Codecov Report
@@ Coverage Diff @@
## master #22899 +/- ##
==========================================
+ Coverage 73.69% 73.72% +0.03%
==========================================
Files 713 714 +1
Lines 94988 95393 +405
==========================================
+ Hits 69998 70332 +334
- Misses 23689 23760 +71
Partials 1301 1301
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
chamikaramj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why we need to make this a callable for Python to recognize it as a type (may be also update the comment here) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there's a better reference on this, but the CPython implementation explicitly considers anything that's callable a type:
https://github.com/python/cpython/blob/d348afa15d5a997e7a8e51c0f789f41cb15cc651/Lib/typing.py#L137-L167
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is actually being relaxed in Python 3.11: python/cpython@870b22b#diff-ddb987fca5f5df0c9a2f5521ed687919d70bb3d64eaeb8021f98833a2a716887
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Optionally, add a comment that this can be removed after we are fully in Python 3.11 (whenever that is).
|
Run Python PreCommit |
chamikaramj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Optionally, add a comment that this can be removed after we are fully in Python 3.11 (whenever that is).
|
@TheNeuralBit Can this be merged ? |
Let me add a comment with an explanation, then I will self-merge. Thanks! |
d62f54e to
dc8408d
Compare
|
Run Python PreCommit |
1 similar comment
|
Run Python PreCommit |
Fixes #22854
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.