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

bug: Wrong parameter order in AnnotableMeta. #3730

Closed
gerrymanoim opened this issue Apr 7, 2022 · 3 comments · Fixed by #3732
Closed

bug: Wrong parameter order in AnnotableMeta. #3730

gerrymanoim opened this issue Apr 7, 2022 · 3 comments · Fixed by #3732

Comments

@gerrymanoim
Copy link
Contributor

The following code which was working as of 3ce4f2a is now no longer working. Reasonably sure it is because of the changes in 088169a around constructing the __signature__ in AnnotableMeta (088169a#diff-1ba248744f86eb25b4e863648c19353294f34c47b637d1fe88d66eccfc1e1119R100-R108).

import ibis.expr.operations as ops
import ibis.expr.rules as _rlz
import ibis.expr.types as ir

class SomeOp(ops.ValueOp):
    a = _rlz.instance_of(ir.TableExpr)
    b = _rlz.optional(_rlz.instance_of(ir.TableExpr), default=None)
    c = _rlz.instance_of(bool)

now raises

Traceback (most recent call last):
  File "/home/gmanoim/tmp/tmp.py", line 5, in <module>
    class SomeOp(ops.ValueOp):
  File "../ibis/common/grounds.py", line 116, in __new__
    signature = inspect.Signature(
  File "../python3.9/inspect.py", line 2817, in __init__
    raise ValueError(msg)
ValueError: non-default argument follows default argument
@cpcloud
Copy link
Member

cpcloud commented Apr 7, 2022

I believe this is intentional so that we conform to Python's signature restrictions. @kszucs would know more.

@kszucs
Copy link
Member

kszucs commented Apr 7, 2022

That wasn't intentional, so this is a valid issue. Thanks @gerrymanoim for reporting!

@cpcloud
Copy link
Member

cpcloud commented Apr 7, 2022

Ah, great! Thanks for the quick fix @kszucs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants