Closed binarygary closed 10 months ago
Elegantly implemented! I left a couple of comments about some minor issues :-)
I wonder what @JasonTheAdams and @borkweb think about this approach.
@nikolaystrikhar Thanks for the quick review! I've made the requested updates. I did guess on the since tag.
@binarygary Since I'm the only one who checked it, I decided to add more comments about small stuff, but so useful when it comes to many brands :-) And one format fix. Didn't want to bother you :D
And I'm not sure if I can approve tbh, I will ping Matt B about it, so he checks.
Regarding the since tag, I wonder how we are going to do it in libraries, @borkweb maybe we want TBD
like we are doing everywhere and then you can update it when you release a new version?
@binarygary @borkweb In the future we might consider optimizing this a bit more. Namely, when checking to see if the dataset already exists, we can create a modified query that is faster than the final update/insert query. Three things come to mind:
SELECT 1
, since we don't actually need to grab the data to see if the row existsLIMIT 1
since we only need to know if there's at least 1 row
This is a simplified implementation of #8. There wasn't any discussion in that issue about whether this is a desired feature or not, but I thought I'd give it a try as an excuse play with slic and this library.
In #8 parameter 1 suggests supporting multiple inserts in a nested array. This does not appear to be available in the CRUD trait or in this library. The second is the columns to match against. The third param is the columns to update on match. I've opted to omit this and update all columns when a match is found.
Usage:
Test coverage has been added for: