Skip to content

Conversation

@colings86
Copy link
Contributor

@colings86 colings86 commented Apr 20, 2017

These methods already exist for ObjectParser. This change adds them to ConstructingObjectParser so they are available when parsing to an object that doesn't have a zero-arg constructor

@colings86 colings86 self-assigned this Apr 20, 2017
@colings86 colings86 requested a review from nik9000 April 20, 2017 16:16
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.

Left some minor things but LGTM in general. Do what you want with my recommendation and merge when you ready. No need for another round of review from me.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Can you declare this method as abstract in AbstractObjectParser so we can inherit the javadoc?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it is "these" marked consumers now.

Copy link
Member

Choose a reason for hiding this comment

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

Can you make move this to a protected method in AbstractObjectParser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this belongs in AbstractObjectParser since it is only useful in conjunction with the Target Class which is an inherent part of the Constructing ObjectParser (and is a private inner class so is not available outside of it)

@colings86 colings86 merged commit 3c7c4bc into elastic:master Apr 21, 2017
@colings86 colings86 deleted the enhance/namedObjectParsing branch April 21, 2017 08:50
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 21, 2017
* master: (61 commits)
  Build: Move plugin cli and tests to distribution tool (elastic#24220)
  Peer Recovery: remove maxUnsafeAutoIdTimestamp hand off (elastic#24243)
  Adds version 5.3.2 and backwards compatibility indices for 5.3.1
  Add utility method to parse named XContent objects with typed prefix (elastic#24240)
  MultiBucketsAggregation.Bucket should not extend Writeable (elastic#24216)
  Don't expose cleaned-up tasks as pending in PrioritizedEsThreadPoolExecutor (elastic#24237)
  Adds declareNamedObjects methods to ConstructingObjectParser (elastic#24219)
  ESIntegTestCase.indexRandom should not introduce types. (elastic#24202)
  Tests: Extend InternalStatsTests (elastic#24212)
  IndicesQueryCache should delegate the scorerSupplier method. (elastic#24209)
  Speed up parsing of large `terms` queries. (elastic#24210)
  [TEST] make sure that the random query_string query generator defines a default_field or a list of fields
  token_count type : add an option to count tokens (fix elastic#23227) (elastic#24175)
  Query string default field (elastic#24214)
  Make Aggregations an abstract class rather than an interface (elastic#24184)
  [TEST] ensure expected sequence no and version are set when index/delete engine operation has a document failure
  Extract batch executor out of cluster service (elastic#24102)
  Add 5.3.1 to bwc versions
  Added "release-state" support to plugin docs
  Added examples to cross cluster search of using cluster settings
  ...
asettouf pushed a commit to asettouf/elasticsearch that referenced this pull request Apr 23, 2017
…#24219)

* Adds declareNamedObjects methods to ConstructingObjectParser

* Addresses review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants