-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Improve Pyramid location documentaion #2073
base: master
Are you sure you want to change the base?
Improve Pyramid location documentaion #2073
Conversation
hmm, actually with the new semantics of if (InBattlePyramid() == 2) they'll get surprising results. I'd personally rather have an error pointing me to the new name. |
Good point. |
Applied :) |
if (CurrentBattlePyramidLocation() != PYRAMID_LOCATION_NONE || InBattlePike() || InTrainerHillChallenge()) | ||
#else | ||
if (InBattlePyramid() | InBattlePike() || InTrainerHillChallenge()) | ||
if (CurrentBattlePyramidLocation() | InBattlePike() || InTrainerHillChallenge()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be the same, otherwise it looks like the difference is significant for the bug fix (I guess in some sense it is, but then it's much easier to miss the |
vs ||
)
if (!InBattlePyramid()) | ||
if (CurrentBattlePyramidLocation() == PYRAMID_LOCATION_NONE) // CurrentBattlePyramidLocation() == PYRAMID_LOCATION_NONE doesn't match |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment left behind
Description
The
InBattlePyramid
name was a bit of a misnomer, as it could return 0, 1 or 2 instead of just 0 and 1.Added an in-line function with the original name to reduce verboseness and potential downstream conflicts.
Discord contact info
AsparagusEduardo