Skip to content

Implement startAfter for RTDB queries #7209

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

Merged
merged 18 commits into from
Jan 14, 2021
Merged

Implement startAfter for RTDB queries #7209

merged 18 commits into from
Jan 14, 2021

Conversation

jmwski
Copy link
Contributor

@jmwski jmwski commented Dec 22, 2020

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

I still need to do a thorough pass on the tests, but I did look through all other source files. Looks pretty good!

@jmwski jmwski assigned schmidt-sebastian and unassigned jmwski Jan 11, 2021
@jmwski
Copy link
Contributor Author

jmwski commented Jan 11, 2021

@schmidt-sebastian Ready for another round of review.

Comment on lines 80 to 81
unsigned long i = [next length] - 1;

Copy link
Contributor

Choose a reason for hiding this comment

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

This suggestion removes the empty line between the related statements in line 82 and 84.

@jmwski jmwski assigned schmidt-sebastian and unassigned jmwski Jan 12, 2021
@jmwski jmwski requested a review from jsdt January 12, 2021 22:30
@jmwski
Copy link
Contributor Author

jmwski commented Jan 13, 2021

@schmidt-sebastian Again, thoughts on merge order here?

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

You can merge as you please :)

@@ -1,4 +1,5 @@
# v7.5.0
- [added] Implmemented `queryStartingAfterValue` for FirebaseDatabase queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in the other PR, it might be worth mentioning the pagination use case here.

if ([next characterAtIndex:i] != [MAX_PUSH_CHAR characterAtIndex:0]) {
break;
}
i--;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
i--;
--i;

From the Google style guide: "Use the prefix form (++i) of the increment and decrement operators unless you need postfix semantics."

withString:MAX_PUSH_CHAR
startingAtIndex:0];
NSString *maxKeySuccessor = [FNextPushId successor:maxKey];
XCTAssertEqualObjects(maxKeySuccessor, [FUtilities maxName], @"");
Copy link
Contributor

Choose a reason for hiding this comment

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

You can skip the last argument (or provide a description).

jmwski and others added 3 commits January 13, 2021 14:00
* Implement `endBefore` for RTDB queries

* Write tests, various other cleanup

* fixup predecessor function

* CHANGELOG.md

* typo

* 2021+ copyrights need LLC

* cleanup

* style

* Update FirebaseDatabase/Sources/Utilities/FNextPushId.m

Co-authored-by: Sebastian Schmidt <[email protected]>

Co-authored-by: Sebastian Schmidt <[email protected]>
@jmwski jmwski changed the title Implement startAfter for RTDB queries Implement startAfter and endBefore for RTDB queries Jan 14, 2021
@jmwski jmwski merged commit b1766c2 into master Jan 14, 2021
@paulb777 paulb777 deleted the jw/start-after branch January 28, 2021 04:21
@firebase firebase locked and limited conversation to collaborators Feb 14, 2021
@jmwski jmwski changed the title Implement startAfter and endBefore for RTDB queries Implement startAfter for RTDB queries Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants