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

Let IsBounded take True value. #3580

Closed
wants to merge 1 commit into from
Closed

Conversation

robertwb
Copy link
Contributor

This is useful for languages like Python that may use this in a conditional statement (or allow assignment from True->1/False->0).

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [BEAM-1234] Fixes bug in ApproximateQuantiles, where you replace BEAM-1234 with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@robertwb
Copy link
Contributor Author

@tgroh

@tgroh
Copy link
Member

tgroh commented Jul 17, 2017

This also makes IsBounded.UNBOUNDED falsy, yes?

I imagine we'll never add other values, but it's worth noting that if we discover one then we'll have to go update a ton of code that depends on only BOUNDED being truthy.

@robertwb
Copy link
Contributor Author

Yes, this makes IsBounded.UNBOUNDED falsy too. We may want to just make this a boolean, but this is at least an improvement.

@kennknowles if you have nay comments.

@kennknowles
Copy link
Member

I like the readability of the enum. TBH I'd rather it was called Boundedness so noone would try to be boolean-ish about it :-)

But I don't feel a need to object or anything.

@tgroh
Copy link
Member

tgroh commented Jul 19, 2017

This seems fine. My preference is mostly aligned (it would be better for this to be either an actual boolean or renamed to Boundedness, but the current state doesn't make anyone happy), but I would classify this as an improvement.

LGTM.

This is useful for languages like Python that may use this in a conditional statement (or allow assignment from True->1/False->0).
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 70.519% when pulling 88289e2 on robertwb:patch-9 into 0064fb3 on apache:master.

@asfgit asfgit closed this in 71196ec Jul 25, 2017
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.

None yet

4 participants