Skip to content

java: columnIndex starts at 1.#1565

Merged
enisoc merged 2 commits into
vitessio:masterfrom
enisoc:java-client
Mar 16, 2016
Merged

java: columnIndex starts at 1.#1565
enisoc merged 2 commits into
vitessio:masterfrom
enisoc:java-client

Conversation

@enisoc
Copy link
Copy Markdown
Contributor

@enisoc enisoc commented Mar 15, 2016

@erzel The first commit is equivalent to the internal CL. The second needs to build on that.


This change is Reviewable

@enisoc enisoc changed the title Address Java readability comments. java: columnIndex starts at 1. Mar 15, 2016
@erzel
Copy link
Copy Markdown
Contributor

erzel commented Mar 15, 2016

Reviewed 8 of 8 files at r1.
Review status: 6 of 9 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


java/client/src/main/java/com/youtube/vitess/client/cursor/FieldMap.java, line 44 [r2] (raw file):
Check the upper-bound as well?


java/client/src/main/java/com/youtube/vitess/client/cursor/Row.java, line 35 [r2] (raw file):
Add a comment that column indices here are 1-based and perhaps also the motivation why.


Comments from the review on Reviewable.io

@erzel
Copy link
Copy Markdown
Contributor

erzel commented Mar 15, 2016

:lgtm:


Review status: 6 of 9 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from the review on Reviewable.io

Approved with PullApprove

@enisoc
Copy link
Copy Markdown
Contributor Author

enisoc commented Mar 15, 2016

Review status: 6 of 9 files reviewed at latest revision, 2 unresolved discussions.


java/client/src/main/java/com/youtube/vitess/client/cursor/FieldMap.java, line 44 [r2] (raw file):
In general this is just a wrapper around fields.get() which already checks bounds and will throw IndexOutOfBoundsException. The main reason for this checkArgument() is to assert that the index transformation done by this method (columnIndex - 1) is reasonable. In other words it asserts only the constraints being added by this method, not all the transitive constraints of methods it calls.


java/client/src/main/java/com/youtube/vitess/client/cursor/Row.java, line 35 [r2] (raw file):
Done.


Comments from the review on Reviewable.io

Anthony Yeh added 2 commits March 16, 2016 14:32
enisoc pushed a commit that referenced this pull request Mar 16, 2016
java: columnIndex starts at 1.
@enisoc enisoc merged commit 4814f50 into vitessio:master Mar 16, 2016
@enisoc enisoc deleted the java-client branch March 16, 2016 22:16
@enisoc enisoc mentioned this pull request Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants