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

'Self' targeting strategy #585

Merged
merged 10 commits into from
May 13, 2024
Merged

'Self' targeting strategy #585

merged 10 commits into from
May 13, 2024

Conversation

tkz00
Copy link
Contributor

@tkz00 tkz00 commented May 8, 2024

Closes [TargetingStrategy] Self

Motivation

Implement new strategy to target the caster unit of a skill.

Summary of changes

New choose_targets function clause with the 'self' type of targeting strategy, it returns the caster unit id.

How to test it?

Set the targeting strategy for one of your unit character's skills to "self", you can do so in the skills.json file, running Champions.Config.import_skill_config() to update the running backend. Then run a battle and verify when the modified skill is cast, it targeted the same unit that casted it. You can check battle.md on how to run battles.

Checklist

  • Tested the changes locally.
  • Reviewed the changes on GitHub, line by line.
  • This change requires new documentation.
    • Documentation has been added/updated.

@tkz00 tkz00 self-assigned this May 8, 2024
@tkz00 tkz00 marked this pull request as ready for review May 8, 2024 20:10
Copy link
Contributor

@lotuuu lotuuu left a comment

Choose a reason for hiding this comment

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

Functionality is great! Tests have a couple of issues though 😔

apps/champions/test/battle_test.exs Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
apps/champions/test/battle_test.exs Show resolved Hide resolved
apps/champions/test/battle_test.exs Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
apps/champions/test/battle_test.exs Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
@tkz00 tkz00 requested a review from lotuuu May 9, 2024 20:44
Copy link
Contributor

@lotuuu lotuuu left a comment

Choose a reason for hiding this comment

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

Tests are still not doing what they are supposed to do! You can check what's going on for the battle in a test by changing the logger config in file tests.exs to:

config :logger, level: :info

This will let you see info logs in tests.

You can run specific tests like:
mix test apps/champions/test/battle_test.exs:749

Also, you marked some comments from my last review as resolved but didn't commit the suggestion/address the comment, be careful there!

apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
apps/champions/test/battle_test.exs Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
apps/champions/test/battle_test.exs Outdated Show resolved Hide resolved
@tkz00 tkz00 requested a review from lotuuu May 10, 2024 19:49
@lotuuu lotuuu mentioned this pull request May 13, 2024
4 tasks
@ncontinanza ncontinanza merged commit 409dba0 into main May 13, 2024
1 check passed
@ncontinanza ncontinanza deleted the targeting-strategy-self branch May 13, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TargetingStrategy] Self
3 participants