search

apa512 / clj-rethinkdb

Eclipse Public License 1.0
205 stars 42 forks source link

117 eqjoin #175

Closed adrienhaxaire closed 7 years ago

adrienhaxaire commented 7 years ago

Hi,

I've written two tests for eq-join, to contribute to the issue 117.

I'm starting with Clojure, so let me know how I can improve my code! I have chosen to put these tests into a separate file because the fixtures are different, I need two tables to test the joins.

If it's OK, I'd like to add the tutorial examples from the official website, also for the inner and outer joins.

Thanks, Adrien

danielcompton commented 7 years ago

Hey, thanks for this! Happy for you to add the tutorial examples from the website for all the join types. Have added some comments inline to the code. Can you also squash all your commits into one?

adrienhaxaire commented 7 years ago

You're welcome! Thank you for your fast answer and the detailed review. I am learning Clojure and RethinkDB, and writing tests is a nice way to experiment and learn.

I will apply your suggestions to have better code, then squash the commits (never done it, another thing to learn!) to have a clean merge. After that, shall I continue on this branch for the other tests, or should I create a new one?

danielcompton commented 7 years ago

After that, shall I continue on this branch for the other tests, or should I create a new one?

Probably easiest to make a new branch.

A good guide for squashing commits is at http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html.

Thanks!

adrienhaxaire commented 7 years ago

Alright, did the code reviews, and squashed the commits into one. Thanks for the feedback.

I think I messed up during my squashing, I pulled from this branch. The right commit is https://github.com/apa512/clj-rethinkdb/pull/175/commits/2fb7e667e31985f0442acee7fe5358cb16aa45cb.

Please let me know if these modifications are okay and how I can fix this squashing thing :)

Oh and by the way, Circle CI complained about a failed test that I didn't touch in build 513. Did it occur before?

adrienhaxaire commented 7 years ago

I saw that the reset function was still opening a connection, I fixed it as well. Squashed and pushed into https://github.com/apa512/clj-rethinkdb/pull/175/commits/e12f74b6d31454922370b208171edcb02c561db7.

I had to pull to be able to push again. Next time, should I fast forward?

Sorry for being so noob with GitHub, thanks for your time!

lenaschoenburg commented 7 years ago

@adrienhaxaire I think you want to do git push --force after squashing the commits. This will overwrite the previous commits and also removes the need to merge the remote branch into your local one.

adrienhaxaire commented 7 years ago

Thank you @dignati! I've reverted the merge then pushed with the --force option, seemed to work!

DonyorM commented 7 years ago

Is this ready to be merged? I would like to build on this pull request to write the tests for the other merge function.

I've started work by fetching this pull request, but I'd prefer not to submit a new PR containing adrienhaxaire's code.

apa512 commented 7 years ago

I'm on my phone now but will check in a couple of hours.

apa512 commented 7 years ago

@DonyorM merged

adrienhaxaire commented 7 years ago

@DonyorM @apa512 thanks!