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

Core: Put an assert for parent region in Entrance.can_reach just like the one in Location.can_reach #3998

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Core: Put an assert for parent region in Entrance.can_reach just like the one in Location.can_reach #3998

merged 3 commits into from
Sep 25, 2024

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Sep 25, 2024 •
edited
Loading

This is how it already works for locations.

Also update some other similar errors to be a bit more verbose

Tested: No

@NewSoupVi
Core: Move connection.parent_region assert to can_reach
434ed0b 
This is how it already works for locations and it feels more correct to me to check in the place where the crash would happen.

Also update location error to be a bit more verbose
@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Sep 25, 2024
@NewSoupVi NewSoupVi added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Sep 25, 2024
@BadMagic100
Copy link
Collaborator

Don't do this, it will break generic ER.

@NewSoupVi NewSoupVi changed the title Core: Move connection.parent_region assert to can_reach Core: Put an assert for parent region in Entrance.can_reach just like the one in Location.can_reach Sep 25, 2024
@NewSoupVi NewSoupVi added is: refactor/cleanup Improvements to code/output readability or organizization. and removed is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. labels Sep 25, 2024
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Sep 25, 2024
BadMagic100
BadMagic100 reviewed Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

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

I also did not test it but with both assertions in place this seems fine.

@NewSoupVi NewSoupVi merged commit e910a37 into main Sep 25, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BadMagic100 BadMagic100 BadMagic100 left review comments

@Berserker66 Berserker66 Berserker66 approved these changes

Assignees
No one assigned
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NewSoupVi @BadMagic100 @Berserker66 @Exempt-Medic