Skip to content

Conversation

greghogan
Copy link
Contributor

Memoize string representations of AbstractID.

Use lookup table for byte-to-hex conversion in StringUtils.

Memoize string representations of AbstractID.

Use lookup table for byte-to-hex conversion in StringUtils.
@tillrohrmann
Copy link
Contributor

Thanks for your contribution @greghogan. The efficiency improvements look really good. The test failures seem to be unrelated.

Would be great if you could add a small test for the StringUtils method.

Apart from that, +1 for merging.

@StephanEwen
Copy link
Contributor

Looks very good.

I am wondering if we can get rid of the toShortString method. What is that one actually used for?

@uce
Copy link
Contributor

uce commented Dec 15, 2015

Yes, that's good to remove. I pointed that out as well when Greg opened the initial issue.

We can either do it as part of this PR or I can do it as a follow up.

@greghogan
Copy link
Contributor Author

@tillrohrmann There exists StringUtilsTest with small test.

One of the TravisCI errors was a timeout and the other killed by the watchdog. Are timeouts being reworked to use RetryOnException or similar?

@uce It looks like toShortString is used for more than debugging and tests so I'd go for a separate PR.

@fhueske
Copy link
Contributor

fhueske commented Dec 15, 2015

Looks good. 1+ for removing toShortString in a separate issue.
I'll merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants