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

Extract the state machines into replaceable modules #3356

Merged
merged 2 commits into from
Oct 3, 2019

Conversation

cedum
Copy link
Contributor

@cedum cedum commented Sep 27, 2019

This PR extracts the existing state machines into replaceable modules.

A while ago I took ownership of some PRs that completely removes the state machines from a few models (#3038, #3039, #3040, #3041).
While I fully agree with the rationale behind those PRs, I think that rewriting manually each dynamically generated state_machines's method might not be ideal.

AFAIK, the state machines in these models don't need to be customized for the majority of the shops (please correct me). Also, having different state-related logic implementations for different models could be confusing.

By discussing this a bit with @kennyadsl, we thought that it might be worth introducing first an intermediary step that lets developers tinker more easily with the existing state machines instead of removing them completely.
This will enable the (few?) shops which require a custom state-related logic to provide their own while keeping a standard implementation for the rest. Hopefully, it could also lead to more ideas 💡addressing the concerns in the original PR (#3038).

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

Allow replacing the existing state machines with custom ones.
This would allow easier customization in stores.
@cedum cedum force-pushed the extract-state-machines branch from b750f6b to 7c4f82c Compare September 27, 2019 17:19
@BenMorganIO
Copy link
Contributor

This is a really good idea 👍

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I really like how this allows customizing the state machine but keeps it simple for stores that do not need to change these things. I'd love to also see some Guides pages update with some simple examples, but it's not blocking. Just let me know if you plan to do so and I'll add a WIP tag to the PR. Thanks, @cedum!

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@cedum this is a very useful change IMHO, thank you 👍

@cedum
Copy link
Contributor Author

cedum commented Oct 1, 2019

I'd love to also see some Guides pages update with some simple examples, but it's not blocking. Just let me know if you plan to do so and I'll add a WIP tag to the PR

@kennyadsl 👍I'll work on it in this PR

@cedum
Copy link
Contributor Author

cedum commented Oct 1, 2019

I've pushed a draft for the documentation (here's the preview).

Instead of describing it for each model, I preferred to add a generic guide under Customizations.
It would be awesome to have some feedback 🙏

@kennyadsl
Copy link
Member

I think it's a perfect starting point, thanks!

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.

None yet

5 participants