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

Fix int/float conversion to bool #2893

Merged
merged 14 commits into from
Jan 22, 2019
Merged

Fix int/float conversion to bool #2893

merged 14 commits into from
Jan 22, 2019

Conversation

srfrog
Copy link
Contributor

@srfrog srfrog commented Jan 14, 2019

This PR changes the logic in Convert to set int/float zero values as false when converting to bool. All other values will be true. Before this change, an int/float value of one (1) would convert to false bool.

Old behavior:

int: convert 1 to bool -> false
int: convert != 1 to bool -> true

float: convert 1 to bool -> false
float: convert != 1 to bool -> true

string: convert "" to bool -> error

New behavior:

int: convert 0 to bool -> false
int: convert != 0 to bool -> true

float: convert 0 to bool -> false
float: convert != 0 to bool -> true

string: convert "" to bool -> false

This change is Reviewable

@manishrjain
Copy link
Contributor

This is a breaking change, which would affect everyone using bools.

Can you add tests for this change?

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

One comment. Can you also fix the empty string -> bool conversion?

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @srfrog and @gitlw)


types/conversion_test.go, line 218 at r2 (raw file):

		{in: Val{Tid: StringID, Value: []byte("False")}, out: Val{Tid: BoolID, Value: false}},
		{in: Val{Tid: StringID, Value: []byte("srfrog")}, fail: true},
		{in: Val{Tid: StringID, Value: []byte("")}, fail: true},

Shouldn't this be false? Empty string is the default in string. False is the default in bool.

Copy link
Contributor Author

@srfrog srfrog left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @gitlw)


types/conversion_test.go, line 218 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Shouldn't this be false? Empty string is the default in string. False is the default in bool.

Done.

@srfrog srfrog removed the request for review from gitlw January 16, 2019 04:02
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Try to make tests go in one line.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @manishrjain)

@srfrog srfrog merged commit ef404eb into master Jan 22, 2019
@srfrog srfrog deleted the srfrog/make_zero_false_again branch January 22, 2019 04:09
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
* fix conversion from int/float to bool to make zero equal to false

* added tests for int/float -> bool conversion

* change convert empty string to boolean false

* update test to account for empty string to bool false

* added truthy value test

* added falsy value test

* simplified tests and added more cases

* refactor tests for legibility and support

* empty binary should be false

* compacted tests a bit except when failure cases are expected

* fixed typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants