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

Clarify XContentParser/Builder interface for binary vs. utf8 values #7367

Merged
merged 1 commit into from Aug 21, 2014

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Aug 21, 2014

Today we have very confusing naming since some methods names claim to
read binary but in fact read utf-16 and convert to utf-8 under the hood.
This commit clarifies the interfaces and adds additional documentation.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2014

LGTM, thanks for cleaning this up and adding documentation...

@jpountz jpountz removed the review label Aug 21, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2014

@jpountz what do you think about this one nocommit?

@jpountz

View changes

src/main/java/org/elasticsearch/common/xcontent/XContentParser.java Outdated
* <ul>{@link XContentBuilder#field(String, byte[])}}</ul>
* </li>
*
* as well as vai their <code>XContentBuilderString</code> variants of the separated value methods.

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 21, 2014

Contributor

s/vai/via/

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2014

@s1monw it feels wrong to me but this has always been this way so I'm afraid some of our code relies on this behavior. Maybe downgrade the nocommit to a TODO and try to address it in a separate change?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2014

++ will do

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2014

@jpountz updated - I think it's ready

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2014

Agreed, lgtm

[PARSER] Clarify XContentParser/Builder interface for binary vs. utf8…
… values

Today we have very confusing naming since some methods names claim to
read binary but in fact read utf-16 and convert to utf-8 under the hood.
This commit clarifies the interfaces and adds additional documentation.

Closes #7367

@s1monw s1monw force-pushed the s1monw:cleanup_xcontent branch to c4bed91 Aug 21, 2014

@s1monw s1monw merged commit c4bed91 into elastic:master Aug 21, 2014

s1monw added a commit that referenced this pull request Aug 21, 2014

[PARSER] Clarify XContentParser/Builder interface for binary vs. utf8…
… values

Today we have very confusing naming since some methods names claim to
read binary but in fact read utf-16 and convert to utf-8 under the hood.
This commit clarifies the interfaces and adds additional documentation.

Closes #7367

s1monw added a commit that referenced this pull request Sep 8, 2014

[PARSER] Clarify XContentParser/Builder interface for binary vs. utf8…
… values

Today we have very confusing naming since some methods names claim to
read binary but in fact read utf-16 and convert to utf-8 under the hood.
This commit clarifies the interfaces and adds additional documentation.

Closes #7367

@clintongormley clintongormley changed the title Clarify XContentParser/Builder interface for binary vs. utf8 values Internal: Clarify XContentParser/Builder interface for binary vs. utf8 values Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Clarify XContentParser/Builder interface for binary vs. utf8 values Clarify XContentParser/Builder interface for binary vs. utf8 values Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.