Closed lucendio closed 4 years ago
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."
OMG!!! I totally don't understand because i took like 2 seconds to breeze over the code but you should like totally share it with us ... tests and all!
@lucendio - Jokes aside, yes contributions with tests would be most welcome. Once you do, we will carve out some time to properly understand your code and tests ... please be patient with us and THANK YOU for helping the community with your willingness to contribute.
@pulkitsinghal,
I added some first tests. Is this what you had in mind? How would you like to proceed?
Thanks @lucendio - we will follow the approach of checking out a new branch with your PR's changes in it and testing the changes. Afterwards we might send our thoughts back over to your side ... or simply put additional commits to save the round trip and merge.
@aquid - do you have the bandwidth for a code review? I performed a brief review and other than rolling back the changes that pertain to nothing more than styling, I don't have strong insights into the functionality yet. We can discuss this further when you or another team member, schedules some time for a joint code review.
My team and I clearly haven't been able to make time for this yet. What to do ... I'll try to think of some way to free ourselves from our day jobs to get this code review started soon.
+1
Does this fall within the "learning goals" for any of you? You can contribute to open-source by helping out here. cc @Mohammed-Aadil or @kamal0808 or @Bhushan001 or @aquid
In our project we had to exposte the bulk API to improve performance of the re-indexing. Is this somewhat useful? Should I provide some tests?