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

Switching SKIP and STOP to exception control flow #183

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kurtbrose
Copy link
Collaborator

agree we shouldn't modify Fill to respect SKIP and STOP.... maybe Match though?

@mahmoud mahmoud marked this pull request as draft August 4, 2020 03:35
@mahmoud
Copy link
Owner

mahmoud commented Aug 4, 2020

Hmm, I think "tests are green" may be a bit of an overstatement. CI is red and many existing tests were changed.

I see what you're getting at though. It's certainly a purer approach. But it does significantly change/break the behavior of SKIP and STOP, both of which have been part of the API for a very long time. They're going from purely sentinel values, controlling the flow from inside the target, to purely specifier types controlling the flow from inside the spec.

Assuming we need to go this direction (and I can think of reasons, but would like to see them demonstrated), the right way to do this would be to have at least one version (but realistically some time window) where we respect the current behavior but warn.

@kurtbrose
Copy link
Collaborator Author

kurtbrose commented Aug 4, 2020 via email

@mahmoud mahmoud changed the title switched SKIP and STOP to exception control flow; existing tests green Switching SKIP and STOP to exception control flow Aug 6, 2020
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