Move brToString(BytesRef) to ToStringUtils
#13068
Conversation
|
@sabi0 i think you should also migrate these 16 classes to use the method from the |
|
That's what I meant with
Did not want to waste time on this in case the suggestion would be rejected. I will push the changes shortly. |
Both in live and commented out code. Also fixed broken quotes wrapping in commented lines.
|
Thank you @cpoerschke! |
There was a problem hiding this comment.
Thank you @sabi0 -- I can't believe how many (badly) forked copies of this method had proliferated.
I just left one naming comment, otherwise this looks awesome. Naming is the hardest part :)
| public static String longHex(long x) { | ||
| char[] asHex = new char[16]; | ||
| for (int i = 16; --i >= 0; x >>>= 4) { | ||
| asHex[i] = HEX[(int) x & 0x0F]; | ||
| } | ||
| return "0x" + new String(asHex); | ||
| } | ||
|
|
||
| @SuppressWarnings("unused") | ||
| public static String brToString(BytesRef b) { |
There was a problem hiding this comment.
Could we rename it to bytesRefToString?
There was a problem hiding this comment.
Or maybe just toString(BytesRef b)?
There was a problem hiding this comment.
I think toString(br) will look confusingly similar to br.toString().
How about format(br)? Or prettyPrint(br) maybe?
(bytesRefToString(br) does look a bit long)
There was a problem hiding this comment.
Shall I rename it to bytesRefToString() ?
There was a problem hiding this comment.
Yes, please do. Better a bit more verbose than cryptic.
| public static String longHex(long x) { | ||
| char[] asHex = new char[16]; | ||
| for (int i = 16; --i >= 0; x >>>= 4) { | ||
| asHex[i] = HEX[(int) x & 0x0F]; | ||
| } | ||
| return "0x" + new String(asHex); | ||
| } | ||
|
|
||
| @SuppressWarnings("unused") | ||
| public static String brToString(BytesRef b) { |
There was a problem hiding this comment.
Maybe also add a javadoc explaining how this differs from BytesRef.toString? E.g. this one does not assume valid UTF-8 but the other one days (I think)?
There was a problem hiding this comment.
I think this method could be the default toString on BytesRef, actually... in most cases when you wish to dump it using toString it's for debugging purposes: an array of bytes would always be there, utf8 as an additional helpful hint... why add another method? Even the fact that this additional method exists points at the need to actually dump BytesRef in a more verbose way...
There was a problem hiding this comment.
I thought of that too. But reckoned that could be too much of a change if all BytesRef's start spitting out text.
I guess the format will have to be changed somewhat to avoid the BytesRef's text "blending in" with the preceding text:
... previous " + bytesRef; // ... previous term [ww xx yy zz]
Also there is an unfortunate side-effect of an exception being thrown for "binary" data.
There was a problem hiding this comment.
Ok. Small steps. We can always redirect and inline this method later on.
|
Please move entry to 9.11.0 |
|
Will take care of this tomorrow. Sorry for delay. |
|
To the other reviewers: I would like to merge this as a first step. To me this looks fine as it removes the code duplication and we have a better method name. Ok? If we remove the static method and instead use I plan to merge this late afternoon German time. |
|
Backport was easy, no conflicts. I also checked with Thanks @sabi0 ! |
I noticed there are
brToString()methods in 16 different classes.What do you think of providing a shared implementation in the
ToStringUtilsclass?If you find the proposal useful I will proceed with removing the other copies.
Otherwise the
ToStringUtilsclass could probably be deleted - the two methods therebyteArray()andlongHex()are not used anywhere.