Thanks for the comments. Yes, it is in progress. I generally keep imports collapsed and never look at them. I hadn't even realized that was the style here. Thanks for pointing it out.
I already changed ITERABLE to COLLECTION. (Originally I thought I would be using Iterable) and removed the parameterType from the signature. I agree with you: I'm not so sure using JUnit parameterized tests is a great idea here. It is unfortunate JUnit doesn't give you more control. Other frameworks (like Spock or TestNG) let you parameterize specific test methods intead of the entire class. I am thinking that once I fix execute and update tests to also use the parameter type, most tests in the QueryRunnerTest will use it, so perhaps it won't be too bad. If you don't like it, what alternative would you suggest? I'm not crazy about the idea of just copying the test methods. I suppose I could just add calls with collections to the existing test methods, but I'm not crazy about that, either. On Thu, May 19, 2016 at 6:40 AM, Benedikt Ritter <brit...@apache.org> wrote: > <moving this to the dev ML> > > Robert Huffman <robert.huff...@gmail.com> schrieb am Do., 19. Mai 2016 um > 15:32 Uhr: > >> Actually, Iterable doesn't work: in fillStatement you need to know the >> size >> and that's not easily obtainable from an Iterable. So I'm using Collection >> instead. >> >> I cloned the project and have QueryRunner.query working already. Basically >> I changed the private query method to take a Collection instead of varargs >> for the parameters. A public method is added that takes a Collection, and >> the public methods that take arrays simply use Arrays.asList on the >> arrays. You can check out the approach here: >> >> >> https://github.com/rhuffman/commons-dbutils/tree/feature/allow-collections-of-parameters-in-QueryRunner >> >> I will take the same approach on update, insert and batch, create a JIRA >> and attach a patch. >> > > I've added a few comments. Looks good to me overall. > > >> >> >> >> >> On Thu, May 19, 2016 at 6:02 AM, Benedikt Ritter <brit...@apache.org> >> wrote: >> >> > Hello Robert, >> > >> > Robert Huffman <robert.huff...@gmail.com> schrieb am Mi., 18. Mai 2016 >> um >> > 19:00 Uhr: >> > >> > > If a prepared statement is built dynamically, with a variable number >> of >> > > parameters, and parameters are collected in a Collection of some sort >> > > instead of an array, usage QueryRunner requires that the collection be >> > > converted to an array first. This means the parameters are iterated >> > twice: >> > > once to convert to an array and once again in >> QueryRunner.fillStatement. >> > > >> > > Would it violate a design decision if methods were added to >> QueryRunner >> > > that took the parameters as an Iterable instead of as varags? It >> should >> > be >> > > straightforward to add such methods and use an Iterable wrapper >> around an >> > > array to have the varargs methods invoke the new methods that take >> > > Iterables. >> > > >> > > I would be happy to submit a patch if this does not violate some sort >> of >> > > design decision I am not aware of and if the implementation approach >> > sounds >> > > reasonable. >> > > >> > >> > Sounds like a reasonable addition, although I'm not sure I understand >> your >> > proposal with the "Iterable wrapper around an array". Feel free to >> create a >> > JIRA and provide a patch/github PR for adding this functionality. >> Further >> > design discussions about this addition should go to the dev mailing >> list. >> > >> > Benedikt >> > >> >
