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

Improve default value for None in text_signature #3066

Merged
merged 1 commit into from
Apr 12, 2023

Conversation

messense
Copy link
Member

xref #2863

@messense messense force-pushed the text-signature-None branch 3 times, most recently from 795afa0 to a4a58f5 Compare March 26, 2023 09:01
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks!

I think there's another test case to consider, which is what happens if the argument is implicitly defaulted to None, as per https://pyo3.rs/v0.18.2/function/signature#trailing-optional-arguments

i.e. this test case:

#[pyfunction]
fn increment(x: u64, amount: Option<u64>) -> u64 {
    x + amount.unwrap_or(1)
}

should have __text_signature__ == (x, amount=None)

@messense messense force-pushed the text-signature-None branch from a4a58f5 to 672f893 Compare March 29, 2023 01:54
@messense
Copy link
Member Author

Thanks for the review, I think I've addressed it now.

@messense messense force-pushed the text-signature-None branch from 672f893 to 57dbc94 Compare March 29, 2023 02:14
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, looks great, sorry I missed your reply earlier!

bors r+

@PyO3 PyO3 deleted a comment from bors bot Apr 12, 2023
@davidhewitt
Copy link
Member

bors r-

@bors
Copy link
Contributor

bors bot commented Apr 12, 2023

Canceled.

@davidhewitt
Copy link
Member

bors r+

@davidhewitt
Copy link
Member

(whoops, looks like bors is fine and I just hadn't noticed another batch is already merging, sorry for the spam 😅)

@bors
Copy link
Contributor

bors bot commented Apr 12, 2023

Build succeeded:

@bors bors bot merged commit a0e285b into PyO3:main Apr 12, 2023
@messense messense deleted the text-signature-None branch April 12, 2023 08:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants