Skip to content

univalue: Remove confusing getBool method #26673

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 10, 2022
Merged

univalue: Remove confusing getBool method #26673

merged 1 commit into from
Dec 10, 2022

Conversation

ryanofsky
Copy link
Contributor

Drop UniValue::getBool method because it is easy to confuse with the UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool, getBool doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.

The getBool method is also redundant because it is an alias for isTrue. There were only 5 getBool() calls in the codebase, so this commit replaces them with isTrue() or get_bool() calls as appropriate.

These changes were originally made by MarcoFalke in #26213 but were dropped to limit the scope of that PR.

@ryanofsky
univalue: Remove confusing getBool method
293849a 
Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.

The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.

These changes were originally made by MarcoFalke in
bitcoin#26213 but were dropped to limit the
scope of that PR.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 9, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK sipa, w0xlt, justinpickering, hebasto, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto
Copy link
Member

hebasto commented Dec 9, 2022

Concept ACK.

There were only 5 getBool() calls in the codebase...

And 4 of them are in tests.

@justinpickering
Copy link

justinpickering commented Dec 9, 2022 •
edited
Loading

ACK 293849a

@sipa
Copy link
Member

sipa commented Dec 9, 2022

utACK 293849a

w0xlt
w0xlt approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

ACK 293849a

hebasto
hebasto approved these changes Dec 9, 2022
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 293849a, also verified that the removed getBool method is not mentioned in any docs:

$ git grep getBool | wc -l
0

furszy
furszy approved these changes Dec 10, 2022
View reviewed changes
Copy link
Member

@furszy furszy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK 293849a

First glance, seems that a good follow-up would be to minimize the isFalse and isTrue methods usage scope as well. Probably only SettingToString and SettingToInt requires them.

@ryanofsky
Copy link
Contributor Author

ryanofsky commented Dec 10, 2022 •
edited
Loading

First glance, seems that a good follow-up would be to minimize the isFalse and isTrue methods usage scope as well. Probably only SettingToString and SettingToInt requires them.

Would recommend looking at #26213 for this. It removes some bad uses of isTrue (all the bad ones IMO)

EDIT: NVM I see you reviewed it already!

@furszy
Copy link
Member

furszy commented Dec 10, 2022

First glance, seems that a good follow-up would be to minimize the isFalse and isTrue methods usage scope as well. Probably only SettingToString and SettingToInt requires them.

Would recommend looking at #26213 for this. It removes some bad uses of isTrue (all the bad ones IMO)

EDIT: NVM I see you reviewed it already!

Actually, I forgot that one hehe. Thanks for the reminder!

@fanquake fanquake merged commit a28fb36 into bitcoin:master Dec 10, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 10, 2022
@fanquake
Merge bitcoin#26673: univalue: Remove confusing getBool method
70af601 
293849a univalue: Remove confusing getBool method (Ryan Ofsky)

Pull request description:

  Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.

  The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate.

  These changes were originally made by MarcoFalke in bitcoin#26213 but were dropped to limit the scope of that PR.

ACKs for top commit:
  justinpickering:
    ACK bitcoin@293849a
  sipa:
    utACK 293849a
  w0xlt:
    ACK bitcoin@293849a
  hebasto:
    ACK 293849a, also verified that the removed `getBool` method is not mentioned in any docs:
  furszy:
    ACK 293849a

Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d
@bitcoin bitcoin locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@furszy furszy furszy approved these changes

@w0xlt w0xlt w0xlt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ryanofsky @DrahtBot @hebasto @justinpickering @sipa @furszy @w0xlt @fanquake