Skip to content
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

Implemented replace_batch() #1652. Definitely plenty of use cases for this. #4768

Closed

Conversation

woodcockjosh
Copy link

Instead of making people compile the query and then replace the "INSERT" string it's better to have this feature.

@narfbg
Copy link
Contributor

narfbg commented Aug 16, 2016

Already has been suggested and rejected - see not just #1652, but also #4452 - and your reasoning in support of it isn't any better.

@narfbg narfbg closed this Aug 16, 2016
@woodcockjosh
Copy link
Author

Just because you don't have any use case for it doesn't mean there isn't any. I wasn't the first person to open an issue and I won't be the last. Many developers will not even bother and just use a sql workaround which is not required if they have this code aND defeats the entire purpose of active record. I think this feature warrants a larger vote by the community.

One use cases involve inserting many rows of user profile settings into a table where it's inefficient to select the rows then update them one by one. There is no other efficient way to both insert and update multiple rows efficiently without replace, or on conflict update.

Consider also creating multiple shopping cart items that the user clicks save and updates multiple quantities at once, you don't want to have to check each one to see which ones changed or got added. What you want is to insert or update all of them in one query, aka batch replace.

If you're a domain registrar and want to update multiple domain owner data at once including adding ones that don't exist you need this.

I could literally think of thousands of use cases.

There's a reason all good ORM solutions in every language and platform include a batch replace. There's no valid reason to not support this, especially when the query builder class cannot be extended. If you allowed users to extend the query builder I would say there is possibly a reason.

We have insert, replace, batch insert, but no batch replace? It's obviously a missing feature. It doesn't make sense, especially when you have a pull request. Isn't the framework supposed to grow and get better based on the needs of the community? How could this possibily make it worse? If you're concerned about supporting it, I can volunteer to make any required changes.

I think you're holding something back that the community actually needs, and I'm not really sure how that benefits the any of us.

@narfbg
Copy link
Contributor

narfbg commented Aug 16, 2016

Just because you don't have any use case for it doesn't mean there isn't any.

You're basing this on my very first comment on #1652 4 years ago. I didn't point you to #4452 for nothing.

And you're twisting my words on top of that.

I wasn't the first person to open an issue and I won't be the last.

Yep, you're the 3rd in the 5 years since I've been involved with the project. This doesn't mean anything.

Many developers will not even bother and just use a sql workaround which is not required if they have this code

I don't see a problem with that.

aND defeats the entire purpose of active record.

We don't have Active Record.

I think this feature warrants a larger vote by the community.

I don't.

One use cases involve inserting many rows of user profile settings into a table where it's inefficient to select the rows then update them one by one.

I'd say it is highly likely that you have a poor database design if you need to do this.

There is no other efficient way to both insert and update multiple rows efficiently without replace, or on conflict update.

You're saying "efficient", but really mean "easy".

Consider also creating multiple shopping cart items that the user clicks save and updates multiple quantities at once, you don't want to have to check each one to see which ones changed or got added. What you want is to insert or update all of them in one query, aka batch replace.

Fair enough - one half-decent use case.

I'm saying half-decent for a reason though - cart contents are temporary, and as such I don't believe they should be stored in a persistent database in the first place.

If you're a domain registrar and want to update multiple domain owner data at once including adding ones that don't exist you need this.

You'll have a very hard time convincing me that this ever happens, even more so with CodeIgniter being the engine underneath.

I could literally think of thousands of use cases.

You're thinking of ways to use it, not of cases that require its usage or where it is overwhelmingly better than an alternative - that's a completely different thing.

There's a reason all good ORM solutions in every language and platform include a batch replace.

ORMs are a different beast; we don't have an ORM.

There's no valid reason to not support this, especially when the query builder class cannot be extended.
If you allowed users to extend the query builder I would say there is possibly a reason.

By your logic so far, there's no valid reason to reject any feature ever. I call bullshit.

Again, please read through #4452, and more specifically this comment.

Besides, everything that adds tens (and in this case hundreds) lines of code just to replace a single keyword is bloat in my book.

We have insert, replace, batch insert, but no batch replace? It's obviously a missing feature.

Features are not collection items.

It doesn't make sense, especially when you have a pull request.

I appreciate that you did make the effort to actually write the code, which few people do. However, that doesn't mean I must merge it.

Honestly, this expectation that all contributions must be accepted is quite annoying.

Isn't the framework supposed to grow and get better based on the needs of the community?

You know, I get similar pleas almost every time I reject a feature ...

Yes, it is supposed to do that. It's also supposed to consider the usefulness and safety of proposed features, whether they fit within its philosophy, guard itself against feature creep, bloat and technical debt, etc.

What you feel is a "need" for you personally may not be one for the community as a whole, and can very often be exaggerating what is actually a "nice to have sometimes", or poor judgement in the sense that you "need" it only because something else you do makes this seem like an ideal tool (see my "poor database" comment above for an example).

How could this possibily make it worse?

Lack of (known) negatives doesn't mean a definite positive.

If you're concerned about supporting it, I can volunteer to make any required changes.

I am also concerned about supporting it, and I would still be a part of my efforts regardless of whether you'd be helping with it or not.

I think you're holding something back that the community actually needs, and I'm not really sure how that benefits the any of us.

Passive-aggressive remarks like this one don't help your case.

@woodcockjosh
Copy link
Author

Well, I guess if that's how you want to respond to new contributors I won't try to contribute anymore. I was trying to help because I really appreciate the work you have done on CI so far. I would be ok if you at least made the query builder class extendable. Could you at least consider that? I don't want just open an issue so you can just reject it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants