Skip to content

Conversation

@jpalumickas
Copy link
Contributor

Opened branch from #208 to fix tests and rebase to master branch.

Can you take a look @brendon ?

@jpalumickas
Copy link
Contributor Author

@brendon There is one test failing, I think it's random fail, can you restart a build for this pull request ?

@jpalumickas
Copy link
Contributor Author

I restarted a build with amend, now it's passing. It was a random fail.

acts_as_list_class.transaction do
lower_item.decrement_position
increment_position
if lower_item.send(position_column) != self.send(position_column)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering with these methods. Do we need the branching here. Could the first branch not handle all situations (i.e. consecutive numbering and non-consecutive?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PoslinskiNet can you answer that ? 😉

Copy link
Contributor

@PoslinskiNet PoslinskiNet Aug 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is possible, but didn't want to change the default behaviour, just handle "non-consequence" branch, so consider that as a refactor :).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, yes that makes sense.

@brendon
Copy link
Owner

brendon commented Aug 19, 2016

Thanks @jpalumickas, I just had a couple of queries. If we can settle those then I think this can be merged :) Closes #208 (replaced by this PR).

@brendon
Copy link
Owner

brendon commented Aug 21, 2016

Just a note. I have another possibly compatibility-breaking change here: #220 so it might make sense to bump the minor version to 0.8.0 to take both of these two changes into account. At least people will then double-think blindly upgrading. We should probably document these changes in the README with an upgrade message.

@swanandp
Copy link
Contributor

Agree on both counts, version bump and doc updates

On Sun, Aug 21, 2016 at 2:13 PM, Brendon Muir [email protected]
wrote:

Just a note. I have another possibly compatibility-breaking change here:
#220 #220 so it might make
sense to bump the minor version to 0.8.0 to take both of these two changes
into account. At least people will then double-think blindly upgrading. We
should probably document these changes in the README with an upgrade
message.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#223 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFjGFYGAyJXtXJuKFzmNnKMrzr_Zkl8ks5qiA-agaJpZM4JoVvT
.

@brendon brendon merged commit f2e9692 into brendon:master Aug 21, 2016
@brendon
Copy link
Owner

brendon commented Aug 23, 2016

0.8.0 has been released and includes this change.

@jpalumickas
Copy link
Contributor Author

Perfect, thanks 👍

@PoslinskiNet
Copy link
Contributor

Nice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants