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

[WIP] No longer store payload in States #275

Closed
wants to merge 1 commit into from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 27, 2024

Overview

We use Payload#payload in State#initialize, but then do not use it again.

This PR still passes payload to initialize, but no longer stores it in an instance variable.

Before

I had kept @payload around to make debugging easier, but in truth, I have never used it to debug.

After

We no longer store State#payload.

We do need Workflow#payload to validate the State#Next values in State#initalize.
If we store/pass Workflow#payload["State"].keys into State#initialize, then this can be dropped.

We do need Path#payload, but this is not a payload but rather a path location.

WIP

This is valid, it is ready, and I like it.
Leaving as WIP to mark it as not a priority.

@agrare
Copy link
Member

agrare commented Aug 27, 2024

@kbrock why remove it though?

Comment on lines -15 to +20
parse_compare_key
parse_compare_key(payload)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the argument is that anything in @payload is duplicated by other ivars I get it, but the fact that we have to pass it in to other methods seems to be contrary to that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only pass at initialization time.
After that, we do not use it.

@miq-bot
Copy link
Member

miq-bot commented Aug 27, 2024

This pull request is not mergeable. Please rebase and repush.

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Aug 27, 2024

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Aug 27, 2024

This pull request is not mergeable. Please rebase and repush.

Overview
---

We use `Payload#payload` in `State#initialize`, but then do not use it again.

This PR still passes `payload` to `initialize`, but drops the `payload` variable.

I kept `payload` around to make debugging easier, but in truth, I have never used it to debug.

Exceptions
---

We do need `Workflow#payload` to validate the state references in `"Next"`.
If we store/pass `Workflow#payload["State"].keys` into child initializers, then this can be dropped.

We do need `Path#payload`, but this is not a payload but rather a path location.
@kbrock
Copy link
Member Author

kbrock commented Aug 28, 2024

update:

  • rebased
  • no longer pass payload to Choice methods.

@miq-bot
Copy link
Member

miq-bot commented Aug 28, 2024

Checked commit kbrock@dd669f1 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
8 files checked, 0 offenses detected
Everything looks fine. 👍

@kbrock
Copy link
Member Author

kbrock commented Aug 28, 2024

Why remove @payload though?

I have a structure in mind:

  • initialize and friends parse the payload into data and actions.
  • run uses that parsed data to act.

If we completely parse at initialization, then we detect most errors upfront and let the user know there are issues.
Alternatively, if we parse at runtime, then we detect the errors at a future time, and if the user is not monitoring workflow.log, then possibly missed that an error even ocured.

So not having payload around means we parse everything upfront.


But Yea, "why remove this?" - this is the reason for keeping this as a WIP from the start.

@kbrock kbrock closed this Sep 18, 2024
@kbrock
Copy link
Member Author

kbrock commented Sep 18, 2024

Giving up for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants