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

Allow default None for optional bools #136

Merged
merged 2 commits into from
Dec 29, 2020

Conversation

notmgsk
Copy link
Contributor

@notmgsk notmgsk commented Jul 20, 2020

Slots of type :bool now support a default of None in the Python messages. Specifically, when a slot has both :type :bool and :required nil, and does not have an explicit :default the resulting Python message will look something like slot: Optional[bool] = None. This allows when to differentiate between whether a false-y value was provided (False) or no value was provided None.

Todo

  • Test

@notmgsk notmgsk requested a review from a team as a code owner July 20, 2020 16:31
@notmgsk
Copy link
Contributor Author

notmgsk commented Jul 20, 2020

Ping @stylewarning @ecpeterson

@ecpeterson
Copy link
Contributor

ecpeterson commented Jul 20, 2020

It's been a while since I've looked at this repo, and I didn't expand the PR to see its global effects, but the local diff all looks reasonable to me. I'd approve if it were up to me.

Does this PR include regenerated python bindings? Did anything change? Did you mean for something to change?

@notmgsk
Copy link
Contributor Author

notmgsk commented Jul 20, 2020

It's been a while since I've looked at this repo, and I didn't expand the PR to see its global effects, but the local diff all looks reasonable to me. I'd approve if it were up to me.

Ignoring the ceremonial "approve PR" button, it is up to you in spirit.

Does this PR include regenerated python bindings? Did anything change? Did you mean for something to change?

It does. No, and no. The intended changes were already merged in a hold-over PR, and this PR just duplicates those changes (and makes them honest).

@ecpeterson
Copy link
Contributor

Ah, good. Thank you for not pinging me on the hold-over PR; I'd have had a fit.

@notmgsk notmgsk force-pushed the 134-none-type-for-optional-bools branch from 619df3e to 0619214 Compare December 21, 2020 15:25
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