search

takezoe / solr-scala-client

Solr Client for Scala
Apache License 2.0
91 stars 43 forks source link

More collections #54

Closed jeffdyke closed 5 years ago

jeffdyke commented 5 years ago

Per https://github.com/takezoe/solr-scala-client/issues/53, i have added a collection method for all calls as well as tidied up BatchRegister to more cleanly call the appropriate method. I found that when sending a null in (i swapped that out for None), it would still use the method that takes a collection, so the NPE still exists. I could be wrong about that portion, but the match felt the safest to me.

I think it's really unfortunate that the JavaClient allows for both instantiations as it leads to incredibly unsafe code, especially since the client is so cheap to create. Personally i'm now creating the client with no collection in the URI and using all the collection methods.

I've tested this thoroughly and will add unit tests if this approach is is accepted.

jeffdyke commented 5 years ago

What would make you comfortable merging this? Or are you now?

jeffdyke commented 5 years ago

Hello @takezoe - A couple things: 1) I'm not very familiar with Mochito so am not having a good time fixing the tests, i'd love some assistance, so then i could write more tests.

2) Would you be willing to publish this as a snapshot if not direct release. I have some ideas about how to make this safer, but don't want to start until we agree on this.

jeffdyke commented 5 years ago

I have published this to my own github b/c i required this functionality, at least to be safer and raise more errors when they occur and show results rather than swallowing them with unit. I have left it under your namespace, so we can continue to share. Somethings will conflict, but i needed to move forward.

takezoe commented 5 years ago

@jeffdyke Sorry for my late reply. I'll try to fix failing tests.

takezoe commented 5 years ago

@jeffdyke I created #55 to fix failed tests (and added some new tests) without last 3 commits of this pull request. Is it good for you? If so, I'm going to merge #55 instead of this, and release the next version (or SNAPSHOT).

jeffdyke commented 5 years ago

@takezoe first thank you. Secondly yes #55 makes more sense as i should have re-branched to self publish.

takezoe commented 5 years ago

OK. I merged #55 and closed this pull request. I'm going to release 0.0.20.

takezoe commented 5 years ago

Released.