jkomoros / boardgame

An in-progress framework in golang to easily build boardgame Progressive Web Apps
Apache License 2.0
31 stars 4 forks source link

Make PropertyReader string-based getters less error-prone #737

Open jkomoros opened 5 years ago

jkomoros commented 5 years ago

For example, in #736, some of the helpers will take string-based property names for fields on given sub-states. But those are brittle if the property name changes.

Moves handle this by having a ValidConfiguration that is checked early on and fails if any of the configured property names don't exist, allowing the GameManager to fail to be created.

Ideally there'd be a way for e.g. string-based property refs to be checked for legality at creation time.

You can imagine a system where GameManager has a CreatePropertyRef(refOptions...) where refOptions is a series of functors like WithGameProperty(name string) or WithDeckIndex(index int). And then it returns an error if that property doesn't exist. You can then have a series of Fetch() interface{} or FetchInt() int properties on the fetcher.

... But really the most important thing is that at GameManager boot up time, we call all of the various string-based property getters and check if they'll fail.

You don't want a ComputedValue getter on a state value to have an int, error signature because only once, at boot, do you want to test, and otherwise it's just annoying to have to do an if err != nil check every time you use it.

So maybe every SubState has a ValidConfiguration() error, which GameDelegate calls at boot up and then if it errors it fails to set up. And then have a convention where any time you fetch a property via its string name, you should actualy create a StatePropertyRef handle to it at startup time and then use StatePropertyRef.Fetch(state) or typed-getters like StatePropertyRef.FetchInt(state) (after checking StatePropertyRef.TypeOf(state))

Ideally we'd also make it hard to do string-based property fetching in the first place, but I don't know how to do that since each PropertyReader has to have the getters be public methods

jkomoros commented 5 years ago

Might be as simple as the StatePropertyRef.Validate(exampleState ImmutableState) error that validates whether the thing referred to by the ref is a valid StatePropertyRef for this item.

And then the convention is that in GameDelegate.BeginSetUp you instantiate those globals and call Validate() and return an error if they error. And document that pattern.

jkomoros commented 5 years ago
jkomoros commented 5 years ago
type ComponentPropertyRef struct {
  //The deck that the component ref is part of
  DeckName string
  //PropName is the proeprty on the ComponentValues, or, if Dynamic is true, on the DynamicComponentValues
  PropName string
  //True if the property is on the dynamic component values, not the component values
  Dynamic bool
}

func (c ComponentPropertyRef) Validate(exampleState ImmutableState) error {

}

func (c ComponentPropertyRef) Fetch(instance ComponentInstance) interface{}, error {

}

//Returns the serialization appropriate for a StackConstraint serialization. e.g. "MyProperty" if MyProperty is unambiguous (on either fixed or dynamic only) or "component.MyProperty" or "dynamic.MyProperty" if ambiguous
func (c ComponentPropertyRef) Serialize() string {

}
jkomoros commented 4 years ago

Actually there's no need for ComponentPropertyRef, since StatePropertyRef already does DynamicComponentValues. Just add StateGroupComponentValues.

Maybe they should just be renamed to ReaderRef, since they're more general.

SanitizationPolicy should have comment changed to make it clear that only GroupGame, GroupPlayer, GroupDynamicComponentValues are provided.

And the TypeOf, PropOrNil, etc shoudl just be on Reader. That's captured in #743.