Skip to content

Conversation

@cbuescher
Copy link
Member

In preparation for being able to parse SearchResponse from its rest
representation, this adds fromXContent to ShardSearchFailure.

@cbuescher cbuescher added :Search/Search Search-related issues that do not fall into other categories >enhancement review v6.0.0-alpha1 labels Jan 19, 2017
@cbuescher cbuescher requested review from javanna and tlrx January 19, 2017 19:14
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a nit, LGTM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we rather use assertToXContentEquivalent ? I am always cautious on comparing json strings (keys ordering etc.) but maybe in this case it's ok

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the toXContent tests that are more a way of checking the intended output is as expected (and more to show a simple, fixed example output in the tests), string comparrison is more intuitive and shouldn't break. We also do this in the other toXContent tests, so I will leave this also to stay consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no objections :)

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a minor comment in tests, otherwise LGTM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); ?

In preparation for being able to parse SearchResponse from its rest
representation, this adds fromXContent to ShardSearchFailure.
@cbuescher cbuescher force-pushed the addParsing-ShardSearchFailure branch from b5a143d to fc6113b Compare January 20, 2017 11:33
@cbuescher cbuescher merged commit 54105f3 into elastic:master Jan 20, 2017
cbuescher added a commit that referenced this pull request Jan 20, 2017
In preparation for being able to parse SearchResponse from its rest
representation, this adds fromXContent to ShardSearchFailure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Search/Search Search-related issues that do not fall into other categories v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants