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

[Bug] Fixed defog not removing the target's Safeguard and Mist #5107

Draft
wants to merge 2 commits into
base: beta
Choose a base branch
from
Draft

[Bug] Fixed defog not removing the target's Safeguard and Mist #5107

wants to merge 2 commits into from
+74 −1

Conversation

Zain-A-Abbas
Copy link

@Zain-A-Abbas Zain-A-Abbas commented Jan 10, 2025 •
edited
Loading

What are the changes the user will see?

Defog will now successfully remove the Safeguard and Mist from the target side.

Why am I making these changes?

Fixes #5086.

What are the changes from a developer perspective?

Added RemoveSafeguard and RemoveMist classes to moves.ts, and has them be called by Defog with both sides set to false. Used RemoveArenaTagsAttr for Defog in moves.ts. Added defog.test.ts as well.

How to test the changes?

Using defog on an enemy who has Safeguard or Mist active would verify the changes work.

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?

@DayKev
Copy link
Collaborator

DayKev commented Jan 10, 2025

So a couple comments:

  • Rather than make a new class for each of these tags, the existing class (RemoveArenaTagsAttr) should probably be updated to properly account for the issue
  • Move and Ability additions/changes/bugfixes need tests added (you can use npm run create-test to create the test file if one doesn't already exist for the relevant move or ability), look at the other tests as an example (if you can find one that handles a similar scenario that's a good place to start).

@Zain-A-Abbas
Copy link
Author

Made the requested changes. Let me know if anything else is needed or if I made a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Defog does not Clear Opposing Safeguard and Mist
2 participants
@Zain-A-Abbas @DayKev