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

[Move] Fully Implement Secret Power #4647

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

Conversation

frutescens
Copy link
Collaborator

@frutescens frutescens commented Oct 12, 2024

What are the changes the user will see?

Secret Power will be fully implemented and will now have its secondary effect.

Why am I making these changes?

Must implement moves...

What are the changes from a developer perspective?

New Move Attribute -> SecretPowerAttr

Screenshots/Videos

Screen.Recording.2024-10-14.at.7.25.44.PM.mov

How to test the changes?

Use Secret Power with a valid Terrain or Biome.

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 considered writing automated tests for the issue?
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)
  • Are the changes visual?
    • Have I provided screenshots/videos of the changes?

Copy link
Collaborator

@Tempo-anon Tempo-anon left a comment

Choose a reason for hiding this comment

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

Can you add a test to see if this is doubled by serene grace but not the rainbow pledge?

src/data/move.ts Outdated Show resolved Hide resolved
src/data/move.ts Show resolved Hide resolved
src/data/move.ts Outdated Show resolved Hide resolved
@Tempo-anon Tempo-anon added the Move Affects a move label Oct 13, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't the test cases be "applies x effect when y biome is active"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that I wanted to use the tests to display corner or edge cases + there's around 25 biomes so it may be too many tests. Open to your thoughts though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems a bit excessive to have ~30 tests for a single move. Ensuring basic functionality + edge cases is probably good enough.

Comment on lines +2883 to +2884
apply(user: Pokemon, target: Pokemon, move: Move, args?: any[] | undefined): boolean | Promise<boolean> {
let secondaryEffect: MoveAttr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
apply(user: Pokemon, target: Pokemon, move: Move, args?: any[] | undefined): boolean | Promise<boolean> {
let secondaryEffect: MoveAttr;
apply(user: Pokemon, target: Pokemon, move: Move, args?: any[]): boolean | Promise<boolean> {
let secondaryEffect: MoveEffectAttr;

Not sure it matters too much, but all the invoked attrs extend MoveEffectAttr.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems a bit excessive to have ~30 tests for a single move. Ensuring basic functionality + edge cases is probably good enough.

src/test/moves/secret_power.test.ts Outdated Show resolved Hide resolved
src/test/moves/secret_power.test.ts Outdated Show resolved Hide resolved
Co-authored-by: NightKev <34855794+DayKev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Move Affects a move
Projects
Development

Successfully merging this pull request may close these issues.

4 participants