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

Boolean expressions of objective prerequisites #927

Merged
merged 30 commits into from
Jan 25, 2023
Merged

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Dec 19, 2022

Closes #795

image

Screenshot from 2022-12-31 22-58-59
image

Tasks

  • Vendor a subset of the boolexpr package
  • New display logic for parallel goals
  • Add a check for whether winning is impossible (due to a "not" prerequisite being achieved)
  • Add a "Lose" dialog analogous to the "Win" dialog
  • Validate no dependency cycles in prerequisites specs
  • Add tests for negative validation result from scenario parsing
  • Web API to inspect status of incomplete, completed, optional, and prerequisite objectives
  • Update all of the multi-goal scenarios to use prerequisites

For follow-up PRs:

@kostmo kostmo force-pushed the objective-prerequisites branch 2 times, most recently from 081da8f to 260d0b8 Compare December 20, 2022 05:02
kostmo added a commit that referenced this pull request Dec 20, 2022
In preparation for #927
mergify bot pushed a commit that referenced this pull request Dec 20, 2022
Towards #795 and in preparation for #927, which adds a lot more Objective-related code.

This is a no-op refactoring, code-movement only.
@kostmo kostmo force-pushed the objective-prerequisites branch 3 times, most recently from 8105de4 to 57bb412 Compare December 28, 2022 04:01
@kostmo kostmo force-pushed the objective-prerequisites branch 4 times, most recently from bf207d0 to 15dba40 Compare December 29, 2022 03:13
@kostmo kostmo force-pushed the objective-prerequisites branch 4 times, most recently from 77f82ab to 4d02e64 Compare January 1, 2023 06:52
@kostmo kostmo requested review from byorgey and xsebek January 1, 2023 07:40
@kostmo kostmo force-pushed the objective-prerequisites branch from 1ec6c00 to 5e682d8 Compare January 1, 2023 07:44
@kostmo kostmo marked this pull request as ready for review January 1, 2023 07:47
@xsebek
Copy link
Member

xsebek commented Jan 1, 2023

@kostmo do you plan to merge this as one commit or squash some of them?

I will try to find some time to test this during the week. 🙂

@kostmo
Copy link
Member Author

kostmo commented Jan 1, 2023

@kostmo do you plan to merge this as one commit or squash some of them?

There are a few worth preserving as distinct commits, but I'll squash the WIP commits.

@byorgey
Copy link
Member

byorgey commented Jan 1, 2023

There are a few worth preserving as distinct commits, but I'll squash the WIP commits.

Note, the way we have Mergify set up, it always squashes merged PRs into a single commit. If you want to preserve things as distinct commits they have to be split out into multiple PRs.

@kostmo kostmo force-pushed the objective-prerequisites branch from 5e682d8 to 5c16451 Compare January 2, 2023 01:06
@kostmo kostmo force-pushed the objective-prerequisites branch from 5c16451 to f988f5e Compare January 2, 2023 01:12
@xsebek
Copy link
Member

xsebek commented Jan 25, 2023

@kostmo thanks for testing my GitHub Action 😄

image

I am really happy it works and sorry it is bothering you. 😅

But at least you don't have to scroll through the comments to find it. 😉

@kostmo
Copy link
Member Author

kostmo commented Jan 25, 2023

@kostmo thanks for testing my GitHub Action smile

image

I am really happy it works and sorry it is bothering you. sweat_smile

But at least you don't have to scroll through the comments to find it. wink

Seems to work well!

@kostmo kostmo force-pushed the objective-prerequisites branch from 48bab41 to 73690cd Compare January 25, 2023 02:30
@kostmo
Copy link
Member Author

kostmo commented Jan 25, 2023

@byorgey I think this is ready now.

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.

🎉 🚀 Looks great!

I noticed a small issue where scenarios with a condition but no goal description etc. (e.g. the Speedrun scenarios) pop up a tiny, empty box at the beginning. We can split that out into a separate issue. (Also, I should add goal descriptions to those Speedrun scenarios.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize representation of dependencies between goals
3 participants