Conversation
There was a problem hiding this comment.
I think it is ok to import XContentParser to make the javadoc shorter if you like.
There was a problem hiding this comment.
Can this be
public static final Aggregator.Parser PARSER;
static {
ObjectParser objectParser = new ObjectParser();
// build the parser
PARSER = (name, parser) -> {
// Stuff
};
}
I think that'd be closer to how we parse other things. Not the same, but closer.
There was a problem hiding this comment.
to be honest, I don't know the reason why this is constructed the way it is. Will see if I can change this.
There was a problem hiding this comment.
I changed this to a more "standard" approach of constructing the parser.
There was a problem hiding this comment.
Same question as for the adjacency matrix one.
There was a problem hiding this comment.
This is more tricky to untangle because of the significanceHeuristicParserRegistry that needs to be passed in. No idea why, but I tried changing this but it gets a bit to involved to do it in this PR I think.
There was a problem hiding this comment.
Fair enough. I wouldn't change the capitalization though.
There was a problem hiding this comment.
I had a quick look and opened #25519 with what I imagine the strategy is. It certainly looks big enough to be worth doing in its own PR.
There was a problem hiding this comment.
I wouldn't change the capitalization though
Thats done because of naming conflicts I otherwise get in https://github.com/elastic/elasticsearch/pull/25486/files#diff-e5532441d96af10726cd20f6b653e3e9R101
I could rename the XContentparser there I guess, but we user capital case for ObjectParser quiet often.
There was a problem hiding this comment.
I wouldn't name it in capital case because it isn't a constant. Otherwise I'm fine with whatever rename you like.
There was a problem hiding this comment.
That makes sense, sorry it took me so long to understand what you mean, but I'm so used to see these parsers as constants (because they usually are) that I didn't see this one isn't.
|
Yes! |
javanna
left a comment
There was a problem hiding this comment.
LGTM thanks @cbuescher for taking care of this
There was a problem hiding this comment.
nit: if we add javadocs maybe we should also say what the method is for?
There was a problem hiding this comment.
I removed this docs, they serve no real purpose
dd496c7 to
7d6ffcc
Compare
QueryParseContext is currently only used as a wrapper for an XContentParser, so this change removes it entirely and changes the appropriate APIs that use it so far to only accept a parser instead.
7d6ffcc to
d1f7a44
Compare
There was a problem hiding this comment.
I had a quick look and opened #25519 with what I imagine the strategy is. It certainly looks big enough to be worth doing in its own PR.
| }; | ||
| private static final ObjectParser<AdjacencyMatrixAggregationBuilder, Void> PARSER = new ObjectParser<>( | ||
| AdjacencyMatrixAggregationBuilder.NAME); | ||
| static { |
There was a problem hiding this comment.
👍 Much more like the others. nice. And less lines!
* master: (52 commits) Include shared/attributes.asciidoc from docs master Fixed page breaks for ICU Collation Keyword Fields Remove QueryParseContext (elastic#25486) [Test] Use a common testing class for all XContent filtering tests (elastic#25491) Tests fix - Significant terms/text aggs (elastic#25499) [DOCS] add docs for REST high level client index method (elastic#25501) Tests: Add Debian 9 (Stretch) to the packaging tests test: Run flush before upgrade and refresh after upgrade. Fix third party audit for repository-hdfs [TEST] Expect nodes getting disconnected quickly testPrimaryFailureIncreasesTerm should use assertBusy to wait for yellow Cleanup network / transport related settings (elastic#25489) Fix repository-hdfs plugin packaging test Remove allocation id from replica replication response (elastic#25488) Adjust BWC version on bad allocation request test Upgrading HDFS Repository Plugin to use HDFS 2.8.1 Client (elastic#25497) Adjust status on bad allocation explain requests Preliminary support for ARM Add doc note regarding explicit publish host Fix typo in name of test ...
QueryParseContext is currently only used as a wrapper for an XContentParser, so
this change removes it entirely and changes the appropriate APIs that use it so
far to only accept a parser instead.