Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: python binding kwargs parsing #5458

Merged
merged 2 commits into from
Dec 26, 2024
Merged

fix: python binding kwargs parsing #5458

merged 2 commits into from
Dec 26, 2024

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Dec 25, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

kwargs parsing is not correct with default pyo3 derive, it will try to parse from attribute instead of dict.__getitem__

Traceback (most recent call last):
  File "C:\Users\Trim21\proj\test\f.py", line 15, in <module>
    print(s3.write("test", b"tc", content_type="text/html"))
AttributeError: 'dict' object has no attribute 'append'

What changes are included in this PR?

Use dict_derive to parse struct from kwargs dict. Didn't find this in pyo3.

another option is to remove #[pyclass] from WriteOptions and remove m.add_class::<WriteOptions>()? which may cause a breaking change and pyo3 doesn't support optional with #[pyo3(item)]

Are there any user-facing changes?

Yes, op.write() not support kwargs.

Copy link
Member

@messense messense left a comment

Choose a reason for hiding this comment

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

LGTM

@messense messense merged commit 93f3aa1 into apache:main Dec 26, 2024
61 checks passed
@trim21 trim21 deleted the fix-kwargs branch December 26, 2024 00:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants