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.

Sign up for GitHub

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

Jump to bottom

Fix objective management upon goal condition exception #1215

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Fix objective management upon goal condition exception #1215

merged 2 commits into from
Apr 18, 2023

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Apr 17, 2023 •
edited
Loading

Fixes #1205.

Bug has existed since #927.

@kostmo kostmo force-pushed the goal-condition-exception branch from cf57fb4 to 386d706 Compare April 17, 2023 05:31
@kostmo kostmo marked this pull request as ready for review April 17, 2023 07:10
@kostmo kostmo requested a review from byorgey April 17, 2023 07:10
byorgey
byorgey approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

This looks fine, and I trust you that it fixes the issue, but can you say a bit more about what the issue was and why this is the right fix? (Either here, and/or, if it may be helpful to others/ourselves in the future, in a source comment.) I'm curious.

@byorgey
Copy link
Member

byorgey commented Apr 17, 2023

Also, what should we do with #1206 ? I'm honestly not quite sure what's going on there.

@kostmo
Copy link
Member Author

kostmo commented Apr 18, 2023 •
edited
Loading

can you say a bit more about what the issue was

I actually had a comment a bit above that explained how the code is supposed to work. But I have now also added an inline comment right at the line where the bug was.

In short, prior to the fold, all "incomplete" goals are "popped" from their list, and then either restored to that list or promoted to "complete" inside the fold. I had forgotten to restore it in the "exception" case.

@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Apr 18, 2023
@mergify mergify bot merged commit 9477c10 into main Apr 18, 2023
@mergify mergify bot deleted the goal-condition-exception branch April 18, 2023 04:54
@kostmo
Copy link
Member Author

kostmo commented Apr 18, 2023

what should we do with #1206 ?

I've closed that PR for now. The test scenario served its purpose to find this bug, but would be more work to design it into the continuous test suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exception in goal condition code is treated as a win
2 participants
@kostmo @byorgey