-
Notifications
You must be signed in to change notification settings - Fork 965
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
Re-Add the DeleteByQuery Functionality #513
Conversation
Reverts 9f3776a and brings the `DeleteByQuery` endpoint up to spec with the current API.
It's in the request body, not the query string.
Awesome, thanks for doing this! I've been snowed under with other projects lately and have been putting this off. The list of params in the spec that you linked are indeed the correct set of params. The docs are unfortunately built from the php doc block comments at the top of each namespace method, which are currently maintained by hand (they used to be automated, broke, hasn't been fixed yet because generally changes are slow and not a big deal). So if you want, you can add the params. If not I'll merge and add the params myself. Up to you! Thanks again :) |
Oh and tests would be covered by the YAML integration tests, although currently Core hasn't ported the tests back. They'll be added at some point, at which the runner on our side will start testing it against a cluster again. :) |
Generated from a bit of scripting around the delete by query api spec.
@polyfractal Added! I just grabbed the delete by query spec json and did a little scripting. Do I need to worry about the text failures here? Looks like master was failing before? |
Any updates here @polyfractal? Do you need anything more from me? |
Hey sorry, busy week. Gonna get to this today :) No need to worry about the test failures, Travis is broken right now (it doesn't like a JSON option, and I haven't figured out how to update it's version of jsonc yet) |
Tested, looks good. Thanks! |
* Re-Add the DeleteByQuery Functionality Reverts 9f3776a and brings the `DeleteByQuery` endpoint up to spec with the current API. * Remove `slice` as a Parameter from DeleteByQuery It's in the request body, not the query string. * Include All the DeleteByQuery Params Generated from a bit of scripting around the delete by query api spec.
Any chance of getting a bugfix release with this functionality in 5.x, @polyfractal ? |
Sure, I'll cut a release today. :) It'll have to be a minor version bump though, since it's a "new" feature. So |
Sounds great, thanks! 👍 |
Sorry for the delay... just tagged |
* Re-Add the DeleteByQuery Functionality Reverts 9f3776a and brings the `DeleteByQuery` endpoint up to spec with the current API. * Remove `slice` as a Parameter from DeleteByQuery It's in the request body, not the query string. * Include All the DeleteByQuery Params Generated from a bit of scripting around the delete by query api spec.
* Re-Add the DeleteByQuery Functionality Reverts 9f3776a and brings the `DeleteByQuery` endpoint up to spec with the current API. * Remove `slice` as a Parameter from DeleteByQuery It's in the request body, not the query string. * Include All the DeleteByQuery Params Generated from a bit of scripting around the delete by query api spec.
Closes #512
Reverts 9f3776a and brings the
DeleteByQuery
endpoint up to spec with the current API.I don't think this is actually covered by tests anywhere?
Addionally, I'm unsure about whether or not I've caught all the allowed params. There are a whole mess of them in the API spec, but some seem like generic settings. As it is, I grabbed the params from the delete by query docs.