Implement asBool() step #3154
Implement asBool() step #3154
Conversation
|
|
||
| None | ||
|
|
||
| `null` values return `null`, numbers evaluate to `true` if non-zero. Strings only accept "true" or "false", any other strings will return `null`. |
There was a problem hiding this comment.
Should mention if the strings are case sensitive or not.
|
|
||
| The `asBool()`-step (*map*) converts the incoming traverser to a boolean value. Numbers evaluate to | ||
| `true` when non-zero (and not `NaN`), `null` evaluates to `null`. Strings are only accepted when | ||
| equal to `"true"` or `"false"` (case-insensitive), otherwise `null` will be returned. |
There was a problem hiding this comment.
Does this mean g.inject("foo").asBool() will return null? Should provide an example of such. Since users will be referencing this documentation more than docs/src/upgrade/release-3.8.x.asciidoc it would be helpful to copy more of the examples from the upgrade doc to this one.
There was a problem hiding this comment.
This contradicts the Exceptions section in the semantics docs.
There was a problem hiding this comment.
Ah yes, semantics docs was a miss.
| "g.Inject(1).AsBool()", | ||
| null, | ||
| null, | ||
| null, |
There was a problem hiding this comment.
Why are the Java, groovy, javascript translations null?
| assertEquals(true, __.__(new BigInteger("1000000000000000000000")).asBool().next()); | ||
| assertEquals(true, __.__("TRUE").asBool().next()); | ||
| assertEquals(false, __.__("false").asBool().next()); | ||
| assertNull(__.__(Double.NaN).asBool().next()); |
There was a problem hiding this comment.
Suggest also adding the following tests:
assertEquals(false, __.__(0.0).asBool().next());
assertNull(__.__(Float.NaN).asBool().next());
assertNull(__.__("").asBool().next());
There was a problem hiding this comment.
assertEquals(false, __.__(-0.0).asBool().next()); would be a good case as well
There was a problem hiding this comment.
Some of these are covered by feature tests so I didn't add here (and vice versa), but I'll duplicate them for better coverage.
| Given the empty graph | ||
| And the traversal of | ||
| """ | ||
| g.inject('true').asBool() |
There was a problem hiding this comment.
Can you add a test to check for case insensitivity?
There was a problem hiding this comment.
Can you add tests for boolean types for example: g.inject(true).asBool() and g.inject(false).asBool()?
| Given the empty graph | ||
| And the traversal of | ||
| """ | ||
| g.inject(-0.0).asBool() |
There was a problem hiding this comment.
Can you add a test for non-zero with decimal ie. 3.14?
There was a problem hiding this comment.
Should override both hashCode and equals methods as the parent default ones should not be used.
There was a problem hiding this comment.
The hashCode function would be identical though? There are no additional variables for the step.
|
|
||
| *Exceptions* | ||
|
|
||
| If the incoming traverser type is unsupported or a string other than "true" or "false" then an `IllegalArgumentException` is thrown. |
There was a problem hiding this comment.
For future thoughts - should the semantics doc be Java centric? not sure what it should be for "Exceptions" but might want to do that differently going forward, particularly for 4.x
| @@ -30,6 +30,35 @@ complete list of all the modifications that are part of this release. | |||
|
|
|||
| === Upgrading for Users | |||
|
|
|||
| ==== Boolean Conversion Step | |||
There was a problem hiding this comment.
Please revise the content here a bit to read a bit more like an announcement rather than the documentation itself. and rather than present all the possible ways it works, maybe just have a good example where it might be helpful and you'd be stuck beforehand.
| ==> IllegalArgumentException | ||
| ---- | ||
|
|
||
| See: link:https://tinkerpop.apache.org/docs/3.8.0/reference/#asBool-step[asBool()-step] |
There was a problem hiding this comment.
is there a JIRA for this?
There was a problem hiding this comment.
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| @StepClassMap @StepAsBool |
There was a problem hiding this comment.
please consider some more practical tests beyond just inject.
| * Parse the incoming traverser as a boolean value. | ||
| * | ||
| * @return the traversal with an appended {@link AsBoolStep}. | ||
| * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#asBool-step" target="_blank">Reference Documentation - asBool Step</a> |
There was a problem hiding this comment.
| * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#asBool-step" target="_blank">Reference Documentation - asBool Step</a> | |
| * @see <a href="http://tinkerpop.apache.org/docs/${project.version}/reference/#asBool-step" target="_blank">Reference Documentation - asBool Step</a> | |
| * @since 3.8.0 |
| } | ||
|
|
||
| @Test(expected = IllegalArgumentException.class) | ||
| public void shouldThrowOnInvalidString() { |
There was a problem hiding this comment.
| public void shouldThrowOnInvalidString() { | |
| public void shouldThrowOnInvalidInput) { |
Where did we land regarding strings such as "not a bool", if that throws an error that should be included here as well.
Implementation of the boolean parsing step
asBool()as outlined in the proposal.VOTE +1