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

[Refactor] [Docs] Minor refactor of move.checkFlags into move.doesFlagEffectApply #5264

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

Conversation

SirzBenjie
Copy link
Member

@SirzBenjie SirzBenjie commented Feb 7, 2025

What are the changes the user will see?

Technically, this fixes an unreported bug where moves called by metronome would improperly not ignore abilities if the user had mold breaker.

Why am I making these changes?

@Wlowscha and I had a long discussion in discord about the fact that the checkFlags method inside moves was confusing.
This class is the ideal place, adhering to the Separation of Concerns design principal, for moves to check whether a flag of theirs can be applied in a given situation (given that the existing structure of the code does not have move instances for moves that are in flight).

However, the name of the method is potentially confusing, and has likely contributed to the method not being used in cases where it probably should be (note, refactoring those uses is not in the scope of this PR).

The signature of the method was also changed in a proactive effort to keep the function's signature from looking like

getBaseDamage(source: Pokemon,
  move: Move,
  moveCategory: MoveCategory,
  ignoreAbility: boolean = false,
  ignoreSourceAbility: boolean = false,
  ignoreAllyAbility: boolean = false,
  ignoreSourceAllyAbility: boolean = false,
  isCritical: boolean = false,
  simulated: boolean = true): number

which is very unwieldly to invoke in cases where you just want to override the default for a later parameter (e.g., in the above case, simulated).

There are likely circumstances where this method would want to be called with additional options to check. For example, in my implementation of Magic Bounce which would like to have its logic for checking the reflectable flag reside here, rather than in the body of the move-effect phase. Using the parameter destructuring pattern keeps code churn small and improves readability. It also has the added benefit of making the arguments at callsites more explicit w.r.t. the parameter they correspond to.

What are the changes from a developer perspective?

moves.checkFlags has been renamed to moves.doesFlagEffectApply. Its signature has similarly been changed to use parameter destructring.
The check for this.followUp when determining whether or not to set the ignoreAbilities global in move-phase has been removed, as the check is now done inside this method. The check logic for MoveFlags.IGNORE_ABILITIES has been modified to return false if the source of the flag was from a move that was being used as a follow up.

Screenshots/Videos

N/A

How to test the changes?

N/A

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • [ ] Have I tested the changes manually?
  • Are all unit tests still passing? (npm run test)
    • [ ] Have I created new automated tests (npm run create-test) or updated existing tests related to the PR's changes?
  • [ ] Have I provided screenshots/videos of the changes (if applicable)?
    • [ ] Have I made sure that any UI change works for both UI themes (default and legacy)?

Are there any localization additions or changes? If so:

  • [ ] Has a locales PR been created on the locales repo?
    • ~~[ ] If so, please leave a link to it here: ~~
  • [ ] Has the translation team been contacted for proofreading/translation?

@SirzBenjie SirzBenjie requested a review from a team as a code owner February 7, 2025 03:06
@SirzBenjie SirzBenjie marked this pull request as draft February 7, 2025 03:14
@SirzBenjie SirzBenjie force-pushed the refactor-move-check-flag branch from 6315dfc to 33957b8 Compare February 7, 2025 03:28
@SirzBenjie SirzBenjie marked this pull request as ready for review February 7, 2025 03:29
Wlowscha
Wlowscha previously approved these changes Feb 7, 2025
@Madmadness65 Madmadness65 added Documentation Improvements or additions to documentation Refactor Rewriting existing code related labels Feb 7, 2025
Xavion3
Xavion3 previously approved these changes Feb 13, 2025
Madmadness65
Madmadness65 previously approved these changes Feb 16, 2025
@SirzBenjie SirzBenjie dismissed stale reviews from Madmadness65, Xavion3, and Wlowscha via 06da392 February 25, 2025 06:52
@SirzBenjie SirzBenjie force-pushed the refactor-move-check-flag branch from 8f4d978 to 06da392 Compare February 25, 2025 06:52
Wlowscha
Wlowscha previously approved these changes Feb 25, 2025
@damocleas damocleas requested a review from DayKev March 5, 2025 15:48
@SirzBenjie SirzBenjie force-pushed the refactor-move-check-flag branch from 7fb32de to 2474553 Compare March 11, 2025 03:07
@SirzBenjie SirzBenjie requested review from DayKev and Xavion3 March 11, 2025 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation Refactor Rewriting existing code related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants