Skip to content
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

Workout zero repeat crash #625

Merged
merged 1 commit into from
Apr 26, 2018

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
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.

None yet

2 participants