Skip to content

Conversation

@gerhardol
Copy link
Collaborator

Zero repeat steps will never finish, #568

Still allow 0 repeats, can be used to skip parts 'dynamically'

@gerhardol gerhardol added this to the Classic_Next milestone Aug 20, 2017
@davidedelvento
Copy link

@gerhardol is this the right PR? We need really 141 commits and change 253 files to fix this bug, dropping FROYO as part of the PR, etc? Or was it a typo and you pushed a branch that was supposed to do something else, which should be merged separately?

@gerhardol
Copy link
Collaborator Author

This PR is based on #624 that is based on #524 So only the last commit is for this PR, the two before for #624 I cannot test in any other way...

@davidedelvento
Copy link

Ah, I see. Can't you merge the other two PRs first and do this one later, just to make things easier to follow?

From a stylistic point of view, why commenting out sections of code instead of deleting them?

@gerhardol
Copy link
Collaborator Author

Commenting out:
I do that when there are better ways to do a change. Could hav added comments too

The Asserts are situations that should not occur, but do. Not new.

I could make a new PR sure, but I believe you will see only the relevant commits when the preceeding PRs are merged.
PRs that build on eachother are harder to follow, but is better than big PRs. If the PRs cannot be separate like this that builds on previous, I believe it is fine.

@davidedelvento
Copy link

Alright, thanks for taking the time to explain.

Still allow 0 repeats, can be used to skip parts 'dynamically'
@gerhardol gerhardol force-pushed the pr/zero-repeat-crash branch from eec1d63 to 210a344 Compare April 25, 2018 23:50
@gerhardol
Copy link
Collaborator Author

Rebased to classic branch

@gerhardol gerhardol merged commit ed6530f into jonasoreland:master Apr 26, 2018
boun pushed a commit to boun/runnerup that referenced this pull request Jul 20, 2018
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.

2 participants