-
Notifications
You must be signed in to change notification settings - Fork 133
feat/improve ruff test coverage #1055
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
|
@CrystalZhou0529 would you mind reviewing the changes I made to |
… in to all methods
5bbb822 to
5f3f7c7
Compare
…ent minimal supported version
…ther changes will come in apache#1043 that is soon to be merged.
|
#1056 contains the follow on work to apply additional rules |
| @overload | ||
| @staticmethod | ||
| def udf( | ||
| input_types: list[pa.DataType], | ||
| return_type: _R, | ||
| volatility: Volatility | str, | ||
| name: Optional[str] = None, | ||
| ) -> Callable[..., ScalarUDF]: ... | ||
|
|
||
| @overload | ||
| @staticmethod | ||
| def udf( | ||
| func: Callable[..., _R], | ||
| input_types: list[pa.DataType], | ||
| return_type: _R, | ||
| volatility: Volatility | str, | ||
| name: Optional[str] = None, | ||
| ) -> ScalarUDF: ... |
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 for sharing this with me! I wasn't aware of this way to overload a function in Python but this is so much cleaner than the previous version. I really appreciate your effort 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.
Lgtm! :)
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
might be a good idea to split out the python version bump into a seperate PR
| readme = "README.md" | ||
| license = { file = "LICENSE.txt" } | ||
| requires-python = ">=3.8" | ||
| requires-python = ">=3.9" |
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.
should we split out this python version bump?
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 don't know why the diff doesn't show, but that's already merged into main
|
|
||
| [[package]] | ||
| name = "datafusion" | ||
| version = "44.0.0" |
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 cant find any reasons why this was removed
Which issue does this PR close?
None
Rationale for this change
During a recent push #1052 , we discovered the need to enable additional ruff checks to catch errors in the repo before they get merged into main. This PR enables many more ruff checks and creates a list of those we still should address or choose to ignore.
What changes are included in this PR?
ALMOST all of the changes are slight formatting. There are some changes to
udafandudfdecorator/functions.Are there any user-facing changes?
These should be transparent to the user.