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

LTTP: Rip Lttp specific entrance code out of core and use Region helpers #1960

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

LTTP: Rip Lttp specific entrance code out of core and use Region helpers #1960

wants to merge 8 commits into from
+38 −34

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

some cleanup

How was this tested?

just unit tests. needs proper testing still

If this makes graphical changes, please attach screenshots.

@ThePhar ThePhar added is: refactor/cleanup Improvements to code/output readability or organizization. affects: core Issues/PRs that touch core and may need additional validation. labels Jul 9, 2023
@el-u
Copy link
Collaborator

el-u commented Jul 10, 2023

Please revisit my feedback from #965, i.e.:

  • implementing connect via super connect
  • correct typing everywhere for addresses and target

el-u
el-u reviewed Aug 24, 2023
View reviewed changes
BaseClasses.py Show resolved Hide resolved
@ScipioWright ScipioWright added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Feb 27, 2024
@Exempt-Medic Exempt-Medic added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Apr 5, 2024
@alwaysintreble
Merge branch 'refs/heads/main' into lttp_entrance
a9818c9 
# Conflicts:
#	worlds/alttp/Rules.py
#	worlds/alttp/UnderworldGlitchRules.py
@alwaysintreble
pokemonrb also uses addresses and target for warps
e994262
@alwaysintreble alwaysintreble removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Jun 1, 2024
BadMagic100
BadMagic100 approved these changes Oct 14, 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 don't own lttp to test but the code looks reasonable and correct

qwint
qwint approved these changes Nov 30, 2024
View reviewed changes
Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

Code is all aligned with my understanding of the APIs (how to use Region.entrance_type, etc.)

purely a code review, did not do any gens/tests/games

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

@el-u el-u el-u left review comments

@BadMagic100 BadMagic100 BadMagic100 approved these changes

@qwint qwint qwint approved these changes

@Berserker66 Berserker66 Awaiting requested review from Berserker66 Berserker66 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
affects: core Issues/PRs that touch core and may need additional validation. 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.
7 participants
@alwaysintreble @el-u @qwint @BadMagic100 @ThePhar @ScipioWright @Exempt-Medic