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

Render all types of literal in the python bindings. #305

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

rfk
Copy link
Collaborator

@rfk rfk commented Sep 29, 2020

I should have rebased #214 before merging, as the new python rondpoint tests interacted badly with the concurrent PR to add support for default arguments. This PR does just enough work to unbreak CI, but doesn't add any tests or similar for the actual use of optional args in python. That work is tracked in #300

@rfk rfk requested a review from a team September 29, 2020 06:40
@rfk rfk force-pushed the python-literal-rendering branch from 277594a to cec3ffc Compare September 29, 2020 06:40
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

:shipit: I'm really pleased to see test_rondpoint.py running now. ✅

Comment on lines +70 to +76
Literal::Boolean(v) => {
if *v {
"True".into()
} else {
"False".into()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Zoinks. Good catch.

Comment on lines +86 to +88
// TODO: render with the intended radix.
Literal::Int(i, _radix, _type_) => format!("{}", i),
Literal::UInt(i, _radix, _type_) => format!("{}", i),
Copy link
Contributor

Choose a reason for hiding this comment

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

#300 will be interested in this.

@rfk rfk merged commit 600882a into main Sep 29, 2020
@rfk rfk deleted the python-literal-rendering branch September 29, 2020 20:29
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 this pull request may close these issues.

2 participants