Pewarta Proxy

📋 Server Request & Proxy Logs

Loading active proxy settings...
Developed by Khairul Adha (adhexa.id)
Mengambil log server...

⚙️ Pengaturan Proxy Target

Skip to content

Fix position when no serial positions - #223

Merged
brendon merged 5 commits into
brendon:masterfrom
jpalumickas:fix-not-serial-positions
Aug 21, 2016
Merged

brendon merged 5 commits into
brendon:masterfrom
jpalumickas:fix-not-serial-positions

Conversation

@jpalumickas

Copy link
Copy Markdown
Contributor

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

Can you take a look @brendon ?

@jpalumickas

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 ? 😉

@PoslinskiNet PoslinskiNet Aug 21, 2016 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

brendon commented Aug 19, 2016

Copy link
Copy Markdown
Owner

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

brendon commented Aug 21, 2016

Copy link
Copy Markdown
Owner

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
Copy Markdown
Contributor

Agree on both counts, version bump and doc updates

On Sun, Aug 21, 2016 at 2:13 PM, Brendon Muir notifications@github.com
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

brendon commented Aug 23, 2016

Copy link
Copy Markdown
Owner

0.8.0 has been released and includes this change.

@jpalumickas

Copy link
Copy Markdown
Contributor Author

Perfect, thanks 👍

@PoslinskiNet

Copy link
Copy Markdown
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