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

Docs: Make an actual LogicMixin spec & explanation #3975

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

NewSoupVi
Copy link
Member

No description provided.

@github-actions github-actions bot added is: documentation Improvements or additions to documentation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Sep 21, 2024
Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Two small comments

docs/world api.md Outdated Show resolved Hide resolved
docs/world api.md Outdated Show resolved Hide resolved
NewSoupVi and others added 2 commits September 21, 2024 19:13
Co-authored-by: Exempt-Medic <60412657+Exempt-Medic@users.noreply.github.com>
Copy link
Collaborator

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

LGTM, but also you told me about it in the first place so

Comment on lines 766 to 767
Only use LogicMixin if necessary. There are often other ways to achieve what it does, like making clever use of
`state.prog_items`, or using event items, pseudo-regions etc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Only use LogicMixin if necessary. There are often other ways to achieve what it does, like making clever use of
`state.prog_items`, or using event items, pseudo-regions etc.
Only use LogicMixin if necessary. There are often other ways to achieve what it does, like making clever use of
`state.prog_items`, using event items, pseudo-regions, etc.

Copy link
Member Author

@NewSoupVi NewSoupVi Sep 25, 2024

Choose a reason for hiding this comment

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

Hmmm, I guess it's fine, it does read better.
The reason I wrote it this way is because in my mind, these are two different "groups" of concepts

  1. Accessing state data directly (state.prog_items, theoretically also stuff like state.reachable_regions or state.blocked_connections)
  2. Using objects from the "normal" classes to express extra logic ("Pseudo" Regions, (Event) Items / Locations)

Copy link
Collaborator

Choose a reason for hiding this comment

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

At the very minimum you need to put the , before etc., or instead of etc. you could write out "or other things" or something like that, I think that would get your way of saying it across better.

@ScipioWright ScipioWright added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Sep 24, 2024
Co-authored-by: Scipio Wright <scipiowright@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: documentation Improvements or additions to documentation. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants