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

Consider changing StatesDesired to use State<Logical, Physical> #52

Closed
4 tasks done
azriel91 opened this issue Nov 20, 2022 · 0 comments
Closed
4 tasks done

Consider changing StatesDesired to use State<Logical, Physical> #52

azriel91 opened this issue Nov 20, 2022 · 0 comments

Comments

@azriel91
Copy link
Owner

azriel91 commented Nov 20, 2022

Currently:

  • StatesDesired is conceptually a Map<ItemSpecId, StateLogical>.
  • StatesCurrent is conceptually a Map<ItemSpecId, State<StateLogical, StatePhysical>>.

The original rationale for this design is to model logical state as something a consumer / user can define or declare, whereas physical state is computed and cannot be known until actual execution.

The downsides to this approach are:

  • Not simple to visually compare both states_current.yaml and states_desired.yaml.

  • The shape of state comparisons cannot be treated uniformly.

    Difference between states.get::<State<StateLogical, StatePhysical>, _> and states_desired.get::<StateLogical, _>:

    let states = resources.borrow::<StatesCurrent>();
    let state = states.get::<State<StateLogical, StatePhysical>, _>(&item_spec_id);
    let states_desired = resources.borrow::<StatesDesired>();
    let state_desired = states_desired.get::<StateLogical, _>(&item_spec_id);

This has caused at least one mix up where I tried to retrieve a StateLogical from StatesPrevious, where I should've been requesting a State<StateLogical, StatePhysical>.


Part of fixing this:

  • Update docs to indicate that StatesSaved is strictly speaking the re-read values from states_current.yaml which may have gone out of date
  • Ensure that StatesCurrent is present iff the current states were discovered in the exact current execution. Change StatesCurrentReadCmd to StatesPreviousReadCmd.
  • StatesDeserializer should be refactored to deserialize StatesSaved and StatesDesired.
  • peace::rt_model_*::Error should use a common variant for States*Deserialize and States*Serialize.
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

No branches or pull requests

1 participant