Skip to content

Allow default None for optional bools#136

Merged
notmgsk merged 2 commits intomasterfrom
134-none-type-for-optional-bools
Dec 29, 2020
Merged

Allow default None for optional bools#136
notmgsk merged 2 commits intomasterfrom
134-none-type-for-optional-bools

Conversation

@notmgsk
Copy link
Contributor

@notmgsk notmgsk commented Jul 20, 2020 •
edited
Loading

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 •
edited
Loading

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

ecpeterson commented Jul 20, 2020

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
@notmgsk notmgsk force-pushed the 134-none-type-for-optional-bools branch from 0619214 to 52754b9 Compare December 21, 2020 15:45
@notmgsk notmgsk merged commit 9b31725 into master Dec 29, 2020
@notmgsk notmgsk mentioned this pull request Dec 29, 2020
Merged
@notmgsk notmgsk mentioned this pull request Feb 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kalzoo kalzoo Awaiting requested review from kalzoo

@braised-babbage braised-babbage Awaiting requested review from braised-babbage

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@notmgsk @ecpeterson

Comments