Skip to content

Add ObjectParser.declareNamedObject (singular) method #53017

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

Merged
merged 2 commits into from
Mar 11, 2020

Conversation

davidkyle
Copy link
Member

Add the method AbstractObjectParser.declareNamedObject (singular) to complement the existing declareNamedObjects (plural). This is a convenience to avoid hacking around with declareNamedObject*s*.

Also a small refactoring to avoid duplicated code in ConstructingObjectParser.declareNamedObject.. methods.

@davidkyle davidkyle added v8.0.0 v7.7.0 :Search/Search Search-related issues that do not fall into other categories >refactoring labels Mar 2, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@davidkyle
Copy link
Member Author

I'm not sure which team owns this code, I've gone for search why not

@davidkyle
Copy link
Member Author

run elasticsearch-ci/bwc

@davidkyle davidkyle force-pushed the single-obj-parser branch from b1ffc1b to 129ba9d Compare March 3, 2020 09:34
@davidkyle davidkyle closed this Mar 6, 2020
@davidkyle davidkyle deleted the single-obj-parser branch March 6, 2020 10:19
@davidkyle davidkyle restored the single-obj-parser branch March 6, 2020 10:44
@davidkyle davidkyle reopened this Mar 6, 2020
@jtibshirani jtibshirani added the :Core/Infra/REST API REST infrastructure and utilities label Mar 6, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/REST API)

@jtibshirani
Copy link
Contributor

jtibshirani commented Mar 6, 2020

I pinged the core-infra team, since they might be interested in reviewing this change.

@jtibshirani jtibshirani removed the :Search/Search Search-related issues that do not fall into other categories label Mar 6, 2020
@rjernst rjernst requested a review from nik9000 March 7, 2020 01:06
@rjernst
Copy link
Member

rjernst commented Mar 7, 2020

@nik9000 Would you please review this?

@davidkyle davidkyle force-pushed the single-obj-parser branch from 129ba9d to f6656fa Compare March 9, 2020 11:08
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -403,7 +425,7 @@ public void declareField(Parser<Value, Context> p, ParseField parseField, ValueT
throw new XContentParseException(p.getTokenLocation(), "[" + field + "] can be a single object with any number of "
+ "fields or an array where each entry is an object with a single field");
}
// This messy exception nesting has the nice side effect of telling the use which field failed to parse
// This messy exception nesting has the nice side effect of telling the user which field failed to parse
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@davidkyle davidkyle merged commit 12613fa into elastic:master Mar 11, 2020
@davidkyle davidkyle deleted the single-obj-parser branch March 11, 2020 10:18
@davidkyle
Copy link
Member Author

Thanks @nik9000

davidkyle added a commit to davidkyle/elasticsearch that referenced this pull request Mar 11, 2020
Add the convenience method AbstractObjectParser.declareNamedObject (singular) to 
complement the existing declareNamedObjects (plural).
davidkyle added a commit that referenced this pull request Mar 12, 2020
Fixes a bug in #53017 where ObjectParser.parseNamedObject would leave the end object 
token unconsumed meaning subsequent fields would not be parsed.
davidkyle added a commit that referenced this pull request Mar 12, 2020
…53395)

Add the convenience method AbstractObjectParser.declareNamedObject (singular) to 
complement the existing declareNamedObjects (plural).
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