[Pagination] add new connection resolver#45167
Conversation
philipp-spiess
left a comment
There was a problem hiding this comment.
Looks great from my side! I'll leave this open for a bit so that someone with more backend experience can also look over it though (*cough* @mrnugget) 🙈
| type resolveOnce struct { | ||
| total sync.Once | ||
| nodes sync.Once | ||
| } |
There was a problem hiding this comment.
What is the reason behind using the sync.Once library function here?
There was a problem hiding this comment.
when calculative pageInfo, we need to count nodes. so if in the query we are fetch both nodes and pageInfo (which is always the case) we will need to compute it twice.
There was a problem hiding this comment.
I don't think these two fields need to live in a separate type, can inline where it's used.
mrnugget
left a comment
There was a problem hiding this comment.
I did a first round of review and mostly commented on code style and quality. It's still a bit hard to judge how far this interface will go without having to be changed, especially since we don't have an example in here.
Requesting changes so that you
- clean up the tests. There's a lot of code in there that can be removed. I bet that whole test file could be 1/4 of its size. (see the one test helper example that I commented)
- add comments to the tests to explain what's being tested where
- make the assertions in the tests clearer: does it matter that we pass in this function, does it not? do we assert what nodes are returned or not? for some of them you want to test the behaviour of your logic here (that the order of the nodes is reverserd, for example, but I don't see that here)
- clean up the
ConnectionResolver(change the int/int32 stuff so it's less casting, add docstrings, remove some superfluous checks, ...)
eseliger
left a comment
There was a problem hiding this comment.
a bunch of nit comments here and there, nothing major. Great work :)
I would love to see this applied to one or two of our existing connections in different parts of the code base that use slightly different patterns, to validate that we can apply this broadly
| type resolveOnce struct { | ||
| total sync.Once | ||
| nodes sync.Once | ||
| } |
There was a problem hiding this comment.
I don't think these two fields need to live in a separate type, can inline where it's used.
mrnugget
left a comment
There was a problem hiding this comment.
Tests and code looks much better! Thanks! There's still a few comments that are unanswered, so I'm waiting for those to be either answered or resolved until approval
e0c0fb5 to
ced63f0
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff e540a5a...f6f161d. No notifications. |
d585832 to
f6f161d
Compare
|
@thenamankumar Looks like auto-merge is bugged/doesn't update when
|
Closes: https://github.com/sourcegraph/sourcegraph/issues/45247
This PR implements a new connection resolver helper, which is planned to be used to implement cursor-based bi-directional pagination throughout our software. This is the first PR which only implements the helper along with its basic unit tests.
The upcoming PRs will include its integration with SavedSearches resolver. The custom sort and filters will be added separately as well.
Test plan
Unit tests are added.
To know more about how this helper will be used, check this out.