Skip to content

Conversation

@josegonzalez
Copy link
Contributor

@josegonzalez josegonzalez commented Jul 31, 2017

I don't believe in tests so I'll leave that as an exercise to the merger.

Closes #23.

ADmad
ADmad previously requested changes Jul 31, 2017
*
* @param \Cake\Datasource\EntityInterface $entity The entity that is going to be saved.
* @return bool
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Can add a second param $number similar to that of TreeBehavior::moveUp(). Same for the moveDown() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I'd have to worry about shifting more entities around, is that fine by you?

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm.. If it makes the code too complicated leave it be :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't think that's quite necessary, and then lends itself to features like "move all the way to top/bottom". Maybe as a separate PR, but this code is probably more useful than the other features.

@ADmad
Copy link
Owner

ADmad commented Jul 31, 2017

I don't believe in tests so I'll leave that as an exercise to the merger.

At least ensure existing tests pass, which don't right now :)

@codecov
Copy link

codecov bot commented Jul 31, 2017

Codecov Report

Merging #24 into master will decrease coverage by 0.76%.
The diff coverage is 92.15%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #24      +/-   ##
============================================
- Coverage     95.08%   94.31%   -0.77%     
- Complexity       41       54      +13     
============================================
  Files             1        1              
  Lines           122      176      +54     
============================================
+ Hits            116      166      +50     
- Misses            6       10       +4
Impacted Files Coverage Δ Complexity Δ
src/Model/Behavior/SequenceBehavior.php 94.31% <92.15%> (-0.77%) 54 <21> (+13)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb827fe...6ccbe01. Read the comment docs.

@josegonzalez
Copy link
Contributor Author

Okay tests pass again, now you just need to add coverage for this :D

@bravo-kernel
Copy link
Contributor

I don't believe in tests so I'll leave that as an exercise to the merger.

Priceless 🤣

@ADmad
Copy link
Owner

ADmad commented Aug 1, 2017

@bravo-kernel It's no surprise then that the code was non functional. The patch has gems like return _movePosition($entity, $direction = '+'); instead of return $this->_movePosition($entity, $direction = '+'); 😝

@bravo-kernel
Copy link
Contributor

I can't believe I am saying this but that's exactly why I appreciate @jadb's insane method of only adding tests better (nowadays), leaving the implementation to the merger :trollface:

@ADmad ADmad dismissed their stale review August 1, 2017 08:18

I fixed shit

@ADmad
Copy link
Owner

ADmad commented Aug 1, 2017

... why I appreciate @jadb's insane method of only adding tests better (nowadays), leaving the implementation to the merger

True TDD :)

@josegonzalez
Copy link
Contributor Author

Close enough.

@ADmad ADmad merged commit eb378c0 into ADmad:master Aug 2, 2017
@josegonzalez josegonzalez deleted the patch-1 branch August 2, 2017 07:01
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.

3 participants