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

Refactor update_all_with_touch #279

Merged
merged 14 commits into from Jul 4, 2017

Conversation

Projects
None yet
3 participants
@ledestin
Contributor

ledestin commented Jun 6, 2017

Hi,

I've refactored update_all_with_touch. If you're curious to know the reasons for my choices, I explain everything in a blog post: https://rubyclarity.com/2017/06/acts_as_list-refactoring-part-3/

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Jun 7, 2017

Collaborator

Hi @ledestin, I think this is ok. @swanandp, did you have any thoughts?

Collaborator

brendon commented Jun 7, 2017

Hi @ledestin, I think this is ok. @swanandp, did you have any thoughts?

@brendon brendon requested a review from swanandp Jun 7, 2017

@swanandp

👍

A word on commits: I am normally not a fan of squashing commits, and I like how you've made small atomic commits which make your refactoring process clear.

However, for such a small diff, having 14 commits will make it future "blaming" a little tedious. In this case, I think it would be beneficial to have them merged together.

If you agree, please squash them together and rebase against master if needed. But if you strongly feel inclined to keep the commits as they are, let me know.

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Jul 3, 2017

Collaborator

Thanks @swanandp, we can actually do this for him with the Squash and merge button if you like?

Collaborator

brendon commented Jul 3, 2017

Thanks @swanandp, we can actually do this for him with the Squash and merge button if you like?

@swanandp

This comment has been minimized.

Show comment
Hide comment
@swanandp

swanandp Jul 3, 2017

Owner

@brendon I am inclined to give him a choice. What do you think?

Owner

swanandp commented Jul 3, 2017

@brendon I am inclined to give him a choice. What do you think?

@brendon

This comment has been minimized.

Show comment
Hide comment
@brendon

brendon Jul 3, 2017

Collaborator

Oh yes definitely. I was just meaning he doesn't have to do the squash himself if he wants it to go that way :)

Collaborator

brendon commented Jul 3, 2017

Oh yes definitely. I was just meaning he doesn't have to do the squash himself if he wants it to go that way :)

@ledestin

This comment has been minimized.

Show comment
Hide comment
@ledestin

ledestin Jul 4, 2017

Contributor

@swanandp @brendon I'm ok with the way it suits you, it's your project after all. Feel free to squash.

Contributor

ledestin commented Jul 4, 2017

@swanandp @brendon I'm ok with the way it suits you, it's your project after all. Feel free to squash.

@swanandp swanandp merged commit 12eb6e9 into swanandp:master Jul 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment