Add Autosense annotation for query dsl testing #18211
Conversation
MaineC
added
>docs
| { | ||
| "query": { |
There was a problem hiding this comment.
I'm not sure if it is ok to confuse indention? I like two spaces and some of the snippets use that but this uses both two and four spaces now.
There was a problem hiding this comment.
Good point - this really was more me being too lazy to reformat than anything else, but I agree it makes a lot of sense to keep these the same within one file.
this adds the autosense annotation to a couple of query dsl docs files and fixes the snippets to work in the tests along the way.
... and range, and terms...
MaineC
force-pushed
the
docs/add_autosense_to_query_dsl
branch
from
6a0522a
to
ab4367c
Compare
| @@ -81,7 +81,7 @@ all documents where the `status` field contains the term `active`. | |||
| This first query assigns a score of `0` to all documents, as no scoring | |||
| query has been specified: | |||
|
|
|||
| [source,json] | |||
| [source,js] | |||
There was a problem hiding this comment.
Do you think we ought to complain for anything [source,json]? It'd be super easy to print a warning/fail the build.
There was a problem hiding this comment.
I'd like us to be consistent here but I hesitate to pull the trigger on that.
There was a problem hiding this comment.
No idea really, though in general I'm in favor of consistency - makes it easier for new comers to adopt existing style.
There was a problem hiding this comment.
In fact the syntax highlighter recognise js, not json, so yes would be good
|
LGTM |
|
Modified the remaining query-dsl docs. Before going ahead and merging I'd appreciate some feedback in particular to the changes made to docs/reference/query-dsl/geo-polygon-query.asciidoc and docs/reference/query-dsl/parent-id-query.asciidoc - examples in both needed a bit of adjustment to get past parsing at all. A second pair of eyes stating that the result is equal to the intent of the example before would be welcome. |
| "points" : [ | ||
| "query": { | ||
| "bool" : { | ||
| "must" : { |
There was a problem hiding this comment.
Does it work if you leave that out?
There was a problem hiding this comment.
Actually I wonder if it'd be better not to use it in the filter context? Like, can you just remove the bool entirely? If it needs the bool we should probably explain why it does. But that can be in another PR I think.
There was a problem hiding this comment.
I had the same thought when making those changes but decided to keep changes minimal and post-pone this to a later PR.
It does parse successfully without being surrounded by the bool query. Not sure if this is a left-over of the filter to query migration. @nknize @clintongormley
There was a problem hiding this comment.
It is only cacheable if it is inside the bool.filter clause, so I would leave it there. (although perhaps delete the bool.must.match_all?
There was a problem hiding this comment.
Makes sense. Will do in a separate PR and apply the same modification to any other reference/query-dsl/geo* docs that have the same structure.
|
I left a comment on the geo-polygon query but overall it looks pretty similar? I think the parent query changes are good. |
Relates to elastic#18211 This reverts commit 20aafb1.
This adds the autosense annotation to a couple of query dsl docs files and fixes the snippets to work in the tests along the way.
Still needed: Switch from autosense to console, convert remaining query dsl docs.
Relates to #18160 / @nik9000