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

The Witness: Change Regions, Areas and Connections from Dict[str, Any] to dataclasses&NamedTuples #4415

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

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Dec 31, 2024

This should make the code more readable, adds more typing, and makes steps towards removing the .txt Logic files (although they will still always be kept as a "source of truth")

I also wanna do Entities, but I thought this would be plenty to review for one PR

Also rename some stuff to be more consistent, mainly change "lambda" to "rule" or "requirement"

Fixes an optional requirement of #4410

Tested:
Running ruff and mypy for correct typing and linting and stuff
Unit tests should hopefully catch any regressions with generation? They are quite extensive nowadays and cover a lot of weird edge cases

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Dec 31, 2024
@NewSoupVi NewSoupVi added the is: refactor/cleanup Improvements to code/output readability or organizization. label Dec 31, 2024
Copy link
Contributor

@Jouramie Jouramie left a comment

Choose a reason for hiding this comment

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

Code LGTM, just some nitpicking.

worlds/witness/data/definition_classes.py Show resolved Hide resolved
worlds/witness/data/definition_classes.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants