magefree / magefree/mage

Fixes to DynamicValue getMessage and toString patterns wrt effect strings

Open
#12,595 34 comments 0 reactions 1 assignee Claimed by @jimga150 View on GitHub
Developers Discussion refactoring
Dominant language
Java
Stars
2.4k
Forks
940
Avg merge
2d 12h
Merged PRs (30d)
160

Description

@xenohedron @JayDi85 @Susucre @theelk801

Proposing that we establish a consistent pattern for how `DynamicValue` subclasses use `getMessage()`. Looking at some previous issues, and digging through the history, it looks like this isn't an issue of a few implementations missing an otherwise consistent pattern, rather the issue is that no consistent pattern seems to exist.

There's effects that dynamically generate text assuming that they will have to add the "for each" or "number of" connectors themselves, and some that don't--and there are `DynamicValue` classes that do the same. This leads to discrepancies like in #12587.

Right now, it seems like each `DynamicValue` has a `toString()` that returns (usually) either "X" or "1", where "X" indicates that the value should use "number of" phrasing (i.e. "+X/+X, where X is equal to the number of ..."), and "1" indicates that the value should use "for each" phrasing ("draws 1 card for each ....")

Some `DynamicValue` subclasses use a multiplier, which can be used or foregone, wherein if the multiplier is used, it implies "for each" usage, otherwise "number of" phrasing is implied.

My proposal:

- Effects should still control the exact phrasing, but that means that sometimes the same `DynamicValue` subclass can be used in a "for each" context AND a "number of" context (see `CardTypesInGraveyardCount` used in both [[Consuming Blob]] and [[Emrakul, the Promised End]]), which means that there needs to be a way for `DynamicValue` subclasses to cater to both.
- `DynamicValue` subclasses should just provide the message in a singular or plural context ("creatures you control" vs "creature you control"), giving maximum flexibility to the owning effect to craft the message.
- I'm open to debate about this, since i'm also tempted to say the `DynamicValue` subclasses should actually own the "number of" and "for each" clauses, but there might be some swath of cards I'm missing that break that pattern such that it will be easier to give control to the effects. Thoughts?

Smaller things, assuming the above is implemented:

- I think that `toString()` should be removed from `DynamicValue`. It looks like its two uses, determining the phrasing and providing the existence of a multiplier, can be wrapped into a single `multiplier` owned by the `DynamicValue` class, that is defaulted to 1 (implying no multiplier)
- In the same vein, i also think that `getSign()` should be changed to just return if the multiplier is negative or not--then no subclass will need to re-implement it.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.