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

Item::apply_dry is not designed to work with non-existent state #196

Open
3 tasks
azriel91 opened this issue Aug 24, 2024 · 0 comments
Open
3 tasks

Item::apply_dry is not designed to work with non-existent state #196

azriel91 opened this issue Aug 24, 2024 · 0 comments

Comments

@azriel91
Copy link
Owner

azriel91 commented Aug 24, 2024

Item::apply_dry will not work for cases like the following:

  1. FileDownloadItem downloads a tar file.
  2. TarXItem extracts the file.

Currently TarXApplyFns::apply_dry just returns state_goal.clone() as the "successful state", rather than attempting to read the file, and returning whether the dry run fails or signal that the returned state is inaccurate because the file to extract doesn't exist yet.

We need to decide to settle on one of the following:

  1. Keep the API as is -- item implementors cannot assume params / data are usable. Errors returned from each Item count as an execution failure.
  2. Change the signature to know which values are RealOrExample::Real(T) vs RealOrExample::Example(T), and item implementors choose to read external resources for Real<T> variants.
  3. Change the implementation of ItemWrapper::ensure_prepare to call state_goal_try_exec when it is for a dry run, and only call apply_dry if state_goal managed to be fetched.

Tasks

  1. Determine the contract, update Item::apply_dry.
  2. Fix ValueResolutionMode::ApplyDry and peace_data::marker::apply_dry::ApplyDry docs.
  3. Update implementations for all *Items.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant