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

Fix _as_string output to only show when format specified #10571

Merged
merged 1 commit into from Apr 14, 2015
Merged

Fix _as_string output to only show when format specified #10571

merged 1 commit into from Apr 14, 2015

Conversation

colings86
Copy link
Contributor

Closes #10284

@jpountz
Copy link
Contributor

jpountz commented Apr 13, 2015

Seeing the fix, it looks like we should either remove ValueFormatter.RAW or enforce valueFormatter to be non-null?

@colings86
Copy link
Contributor Author

@jpountz Good point. I tend to agree. I had a look into removing ValueFormatter.RAW and I'm not sure that's the way to go since if we remove it we will end up just recreating what it is doing inside if (valueFormatter == null) statements which feels like a step backwards to me.

I also had a look at enforcing that the valueFormatter is not null. This should be ok providing that we are ok with using ValueFormatter.Raw in the case where we cannot determine the valueType for the field? (see here). WDYT?

@jpountz
Copy link
Contributor

jpountz commented Apr 14, 2015

@colings86 It sounds good to me!

@colings86
Copy link
Contributor Author

@jpountz is it worth applying the fix as is to 1.5.2, 1.6, and 2.0 and then work on a new PR to force the valueFormat to be non-null from 2.0 onwards? I am a bit worried about maintaining backwards compatibility if we force the ValueFormat to be non-null as there are a number of cases where we currently set the ValueFormat, ValueFormatter and/or ValueParser to null.

@jpountz
Copy link
Contributor

jpountz commented Apr 14, 2015

@colings86 +1 I'm fine with merging this PR if we also commit to clean it up in 2.0.

@colings86 colings86 merged commit 82df50a into elastic:master Apr 14, 2015
@colings86 colings86 deleted the fix/10284 branch April 20, 2015 14:27
@clintongormley clintongormley changed the title Aggregations: Fix _as_string output to only show when format specified Fix _as_string output to only show when format specified Jun 7, 2015
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.

Aggregations: formatted string values are always returned in 1.5
3 participants