Skip to content

Composite State Updates#278

Open
jimPruyne wants to merge 1 commit intomainfrom
pruyne/composite-state-updates
Open

Composite State Updates#278
jimPruyne wants to merge 1 commit intomainfrom
pruyne/composite-state-updates

Conversation

@jimPruyne
Copy link
Contributor

This adds some updates to how the composite states are handled. It changes the protocol for generating the flow definition. It supports setting a prefix on a composite state which is pre-pended to each state generated. To do this, the get_flow_definition implementation calls out to a separate method which generates the composite flow at the state model level rather than at the flow definition dict level.

This also specs the method of setting next on a composite state. There's more than can probably be done in this space, but it does seem to provide the minimum necessary functionality.

Also includes an example state for using Choice to optionally skip a state. This maybe shouldn't be in this package (e.g. in gladier-tools instead), but it does provide an example (and testable) of the use of BaseCompositeState within the main gladier package which probably makes sense to have in the main gladier package.

@jimPruyne jimPruyne requested a review from NickolausDS August 24, 2023 06:49
@jimPruyne jimPruyne force-pushed the pruyne/composite-state-updates branch from 5ceb726 to 031ec9c Compare August 24, 2023 06:56
)

def get_flow_definition(self) -> JSONObject:
start_state = self.construct_flow()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If construct_flow is called here, does it need its own method?

)
flow_definition["States"] = new_states

return flow_definition
Copy link
Collaborator

Choose a reason for hiding this comment

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

The state renaming code might be nice to have in its own separate method for clarity if people are running super().get_flow_definition() a lot. I'm not sure if we'd ever want to call only the state naming functionality out-of-band, but it could possibly be useful. Maybe call it generate_state_names()?

This adds some updates to how the composite states are handled. It
changes the protocol for generating the flow definition. It supports
setting a prefix on a composite state which is pre-pended to each
state generated. To do this, the `get_flow_definition` implementation
calls out to a separate method which generates the composite flow at
the state model level rather than at the flow definition dict level.

This also specs the method of setting next on a composite
state. There's more than can probably be done in this space, but it
does seem to provide the minimum necessary functionality.

Also includes an example state for using Choice to optionally skip a
state. This maybe shouldn't be in this package (e.g. in gladier-tools
instead), but it does provide an example (and testable) of the use of
BaseCompositeState within the main gladier package which probably
makes sense to have in the main gladier package.
@jimPruyne jimPruyne force-pushed the pruyne/composite-state-updates branch from 031ec9c to 6964380 Compare August 27, 2023 01:49
@NickolausDS
Copy link
Collaborator

@jimPruyne Did you have more updates you wanted to add here?

@jimPruyne
Copy link
Contributor Author

@NickolausDS Just coming back to this. I'll take a look at your comments and see what updates we can make to push this forward. Thanks for the feedback.

@ravescovi
Copy link
Contributor

@NickolausDS @ryanchard Can we close this due to inactivity?

@NickolausDS
Copy link
Collaborator

It looks like this is an incremental update to Jim's old stuff. I'll take a look at it again this week.

I really need to go through Jim's old experimental updates. Those need to be given a full evaluation, and either used in some way or thrown out in favor of something else. Right now they're just kinda sitting.

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

Successfully merging this pull request may close these issues.

3 participants