Fix elasticsearch tests#697
Conversation
elasticsearch-py recommends only using kwargs due to the potential for positional args to change order. There's no need for us to maintain an exhaustive matrix for order of arguments when it's an unsupported use-case upstream.
|
@beniwohli I'd prefer to have a review on this since it makes some executive decisions, but I want to get the tests green so I'm going to merge it. When you're back next week, I would still appreciate a review of this, and can make any changes you recommend at that point. |
|
@basepi I had a cursory look earlier today, looks good! I'll have another look tomorrow |
beniwohli
left a comment
beniwohli
left a comment
There was a problem hiding this comment.
just a couple of doc nitpicks :)
| * Django 1.8, 1.9 and 1.10 | ||
| * Flask < 1.0 | ||
|
|
||
| In addition, as of this release we no longer support positional args for |
There was a problem hiding this comment.
Wondering if we should tone down this a little bit, it sounds like things would break if the user uses positional args.
Something like
In addition, as of this release we only supported capturing extended information on elasticsearch queries when using keyword arguments with the elasticsearch-py API. This is in keeping with the
https://elasticsearch-py.readthedocs.io/en/master/api.html#api-documentation[upstream policy]
of positional arguments being unsupported. {pull}697[#697]
| * the query string (if available) | ||
| * the `query` element from the request body (if available) | ||
|
|
||
| We recommend using keyword args only with elasticsearch-py, as recommended by |
There was a problem hiding this comment.
| We recommend using keyword args only with elasticsearch-py, as recommended by | |
| We recommend using keyword arguments only with elasticsearch-py, as recommended by |
3 participants
What does this pull request do?
Fixes elasticsearch tests (failing as of the release of 7.5.1), and removes some brittleness around argument ordering, since that API is dynamically generated and order is not guaranteed.
Also removed our extensive matrix tracking the position of the
bodyargument.elasticsearch-pyactually doesn't support non-kwarg use of the API, they are only args because there's no way in py2 to have a required kwarg. Made a note in the docs and updated the tests to match.