Skip to content
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

Make writePrimitive*() and readPrimitive*() methods public. #5710

Merged
merged 1 commit into from Apr 7, 2014
Merged

Make writePrimitive*() and readPrimitive*() methods public. #5710

merged 1 commit into from Apr 7, 2014

Conversation

aleph-zero
Copy link
Contributor

These utility methods are useful for client code to read/write arrays of
primitive types.

These utility methods are useful for client code to read/write arrays of
primitive types.
@aleph-zero aleph-zero merged commit fb338ef into elastic:master Apr 7, 2014
@uboness
Copy link
Contributor

uboness commented Apr 7, 2014

@aleph-zero maybe jumping a bit too late here, but why not just name the methods writeLongArray & readLongArray? as far as I can see we don't have dedicated methods for non-primitive numeric arrays... would make things less verbose

@aleph-zero
Copy link
Contributor Author

@uboness The read/writePrimitive*Array() methods were used internally by these classes and I thought it better to not go refactoring code that didn't need it.

@uboness
Copy link
Contributor

uboness commented Apr 7, 2014

but that's the point... you're now exposing it to the rest of the system, so renaming the methods now is a small change (later loads of classes will be touching these methods)

@aleph-zero
Copy link
Contributor Author

Okay @uboness I'll open a new issue and change the names.

@s1monw
Copy link
Contributor

s1monw commented Apr 7, 2014

@aleph-zero just change it here? that's just fine a PR is kind of an issue?

@aleph-zero aleph-zero deleted the internal-issue-113.1 branch October 21, 2014 23:36
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.

None yet

5 participants