-
Notifications
You must be signed in to change notification settings - Fork 826
MagicColor: add LandType to Color #9570
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
base: master
Are you sure you want to change the base?
Conversation
|
Perhaps call the field "basicLandType" if Colorless doesn't point to Wastes? Might make sense at some point to split some of these constructor parameters out to EnumMaps. The signature is getting a little cluttered. |
|
Is there any rationale with not making these saMana.hasParam("ReplaceOnly")where this could have been an enum. If you need a display name, you can also do: public enum BasicLandType {
PLAINS("Plains"),
ISLAND("Island"),
SWAMP("Swamp"),
MOUNTAIN("Mountain"),
FOREST("Forest");
private final String displayName;
BasicLandType(String displayName) {
this.displayName = displayName;
}
public String getDisplayName() {
return displayName;
}
} |
I'm not sure the example here illustrates the potential upside from such an enum. I don't think we have too many parameters or fields that we'd want to be exclusive to basic land types. |
|
The main upside I see is discoverability. With something like saMana.hasParam(Param.REPLACE_ONLY), autocomplete makes it much easier to see what parameters are valid. With raw strings, unless you already know the exact value, it’s hard to tell what hasParam can take without searching the codebase. Note: this is just an example to explain why I think |
|
Who is going to be responsible for converting all of the scripts into enums? And then keeping those up to date when new scripts are written? |
|
the enum discussion is not part of this MR |
|
Also part of #9532 |
Part of #8682 and #9532
This adds the basic land type to ColorEnum
to be used for later changes