Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add special encoding of json array elements #3368
feat: Add special encoding of json array elements #3368
Changes from 49 commits
32c2440
53cb4cb
f703e3a
403c587
1c059fb
c784f61
9855f30
0faa02b
92f7958
516d290
af5eba2
3dcb838
e27d5db
7e00694
40341a0
61a7b90
32ef7bc
fc0eb2b
70f8651
cdb9d34
adb71d4
bb67d2f
8f24c04
279bb69
343f5fc
a56d3bf
b31c6c0
69a429b
74605db
efef1b1
85b5e50
c181d9e
669cc85
122bc24
8308672
f237f2f
575a469
edb8386
b6c6da5
1908a18
507ad20
5bd11d8
10d3e30
e6153ce
213bba5
29a0c7f
d20e921
e63fd8c
71dad5d
3acb53d
def1754
528ee75
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 69 in client/json.go
Codecov / codecov/patch
client/json.go#L68-L69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: Please note that this is the index path only, and does not represent the path of the base (datastore-doc) data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I said earlier, although at the moment it's used only by the indexer, it is not index-specific, hence doesn't need to mention it.
And there no point in saying anything about storage, as it's irrelevant. It says "in the JSON tree".
I thought we agreed that TODOs are going to be used only if a reviewer considers the code (or absence of it) a blocker. I don't think any request for comment or documentation can ever be considered a blocker. To me it feels like we failed the experiment at it's very start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very strongly disagree with this. Anyone can write greenfield code that gets the machine to do what they want. The hard, and valuable, bit is writing maintainable code that it readable by your peers, including your future self. Code documentation is a valuable tool when it comes to improving code readability, and IMO reviewers, with their fresh, unfocused eyes are far better judges than authors when it comes to guessing where information is missing.
The fact that there is still confusion around what this function and it's types represent suggests very strongly to me that the code is not self describing and requires further documentation in order for maintainers to safely and efficiently maintain it.
I think perhaps a large contribution to the continued confusion here is a name clash! The existing code/documentation uses the word
index
a lot, and most of the time it is not referring to secondary indexes (the only place where the code is consumed), but to the index of elements in an array.It is further confused by the magic
0
, that forms part of the encoded result of this path, that has the appearance of an array-location, yet is not. The existence/behaviour of this confuses the example in the documentation forJSONPathPart
for myself, although without looking into the codebase/raw-store this problem will not affect users.Perhaps we can reduce this confusion by:
index
in the documentation forJSONPathPart
, for example perhaps talk about thelocation in the array
instead ofarray index
.JSONPathPart
to avoid talking about the first index, so that we avoid the ambiguity of the hardcoded0
. Perhaps add a second element to the example array and talk about that instead. And/or note the existence of the magic0
in that documentation and what it represents (that might be out of place though).GetPath
noting that it is currently only used internally for the encoding of secondary indexes (please make sure you include the wordsecondary
, to minimise the whole index vs index confusion).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think the current documentation is relevant and precise enough. This is not index specific and not related to datastore keys so I don't think its relevant to state that it does not represent a datastore doc path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewers are free to put
todo
on whatever they feel warrants it, reviewers should be more strict with what they mark astodo
currently. As I said, its on the author to be able to declare thetodo
out of scope (importantly, this doesnt mean its wrong or shouldnt be done, just out of scope of this PR).For this particular instance, a
todo
requesting a comment/documentation is a very small request and would take a total of 30 second to do. Unless you feel strongly that adding this comment is blatantly wrong or counter productive (which I don't personally see how it would be in this case).Check warning on line 248 in client/json.go
Codecov / codecov/patch
client/json.go#L248