-
Notifications
You must be signed in to change notification settings - Fork 16
Delete on plural endpoints #534
Delete on plural endpoints #534
Conversation
fe99579
to
f013a55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good :)
However we can't merge without associated tests for these new features
@paullaffitte do you need any help? Let us know 🙏 ! |
@leplatrem Sorry I was on vacation, I think it's okay, maybe I'll need help for the tests but I'll let you know. I'll try to finish it this week-end. |
I'm done with writing tests, but I can't seems to find how to properly run the integration tests. |
41dd2f2
to
f0e72c1
Compare
@paullaffitte You'll need to be in a python venv to run the integration tests. (Alternatively you can The following should work with Python 3 on Linux: $ python3 -m venv .venv
$ source .venv/bin/activate
(.venv) $ pip install kinto kinto-attachment
$ npm run test |
f0e72c1
to
a0e9b2f
Compare
That's very nice that there is some automated tests now. If they were there at the time I started the PR, it would be already merge maybe haha. Anyway, sorry about the time I took to complete it, I was overwhelmed by work/studies/personal projects and I kind of forgot about this PR. I rebased my branch today and updated what should be updated. I also have tested it locally but I guess that the automated tests will show us if my work is good or not. |
a0e9b2f
to
70c8e3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Thanks for coming back to this!
I reported a few nits to address and then we're good to merge!
That's very nice that there is some automated tests now. If they were there at the time I started the PR, it would be already merge maybe haha.
What do you mean? This repo had tests since its first commit :) https://github.com/Kinto/kinto-http.js/tree/87c681c82a
I maybe just didn't remember about it, I was pretty sure that I had to run the tests locally. I will do the requested changes during this week. |
Looks like the tests on firefox failed for session issue rather than because of my code (that I didn't change since last successful pipeline anyway, so it would makes sense). |
All good now :) I'll review this over the next few days. Thanks again for contributing! |
@paullaffitte Thank you so much for contributing, and hanging in there while we reviewed this. I really appreciate your patience and your effort in getting this ready to merge! 🎉 🎉 |
It implements deleteRecords, deleteCollections and deleteBuckets, with the same filtering as listRecords. #95