Change Bound's before to inclusive #2981
Conversation
This is supposed to reduce confusion as to what means for startAt() and endAt().
|
cc @wu-hui |
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (533749c0) is created by Prow via merging commits: d56db2a 3e89c25. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (533749c0) is created by Prow via merging commits: d56db2a 3e89c25. |
|
This breaks the integration tests due to some logic change in regards to Bound.sortsBeforeDocument() |
| @@ -976,8 +977,8 @@ public void testEncodesLimits() { | |||
| public void testEncodesBounds() { | |||
| Query q = | |||
| Query.atPath(ResourcePath.fromString("docs")) | |||
| .startAt(new Bound(asList(Values.refValue(databaseId, key("foo/bar"))), true)) | |||
| .endAt(new Bound(asList(Values.refValue(databaseId, key("foo/baz"))), false)); | |||
| .startAt(bound(true, refValue(databaseId, key("foo/bar")))) | |||
There was a problem hiding this comment.
optional: include code comment for true value here and throughout
| return inclusive ? comparison <= 0 : comparison < 0; | ||
| } | ||
|
|
||
| /** Returns true if a document sorts after a bound using the provided sort order. */ |
There was a problem hiding this comment.
readability request: can you include this comment somewhere in the class to save future us from going through this again?
There was a problem hiding this comment.
Done (added an abbreviated version to isInclusive).
| @@ -167,24 +167,35 @@ private int decodeLimit(JSONObject structuredQuery) { | |||
| } | |||
| } | |||
|
|
|||
| private Bound decodeBound(@Nullable JSONObject bound) throws JSONException { | |||
| private Bound decodeStartAtBound(@Nullable JSONObject bound) throws JSONException { | |||
There was a problem hiding this comment.
Hm. I wonder if we can offload the duplicated logic into decodePosition, or if that just gets us back to the same level of misdirection.
There was a problem hiding this comment.
We would need to somehow convey whether this is a start or end bound.
| @@ -318,12 +318,14 @@ public String getCanonicalId() { | |||
|
|
|||
| if (startAt != null) { | |||
| builder.append("|lb:"); | |||
| builder.append(startAt.canonicalString()); | |||
| builder.append(startAt.isInclusive() ? "b:" : "a:"); | |||
There was a problem hiding this comment.
optional: move this ternary into a method on Bound.
There was a problem hiding this comment.
The result is flipped depending on whether this is a startAt or endAt cursor, hence I pulled it out.
| @@ -176,7 +178,7 @@ public void addIndexEntries(Document document) { | |||
| if (fieldIndex == null) return null; | |||
|
|
|||
| Bound lowerBound = target.getLowerBound(fieldIndex); | |||
| String lowerBoundOp = lowerBound.isBefore() ? ">=" : ">"; // `startAt()` versus `startAfter()` | |||
| String lowerBoundOp = lowerBound.isInclusive() ? ">=" : ">"; | |||
There was a problem hiding this comment.
this cleanness makes everything worth it.
This is supposed to reduce confusion as to what
beforemeans for startAt() and endAt().