Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workfow] Rename current MarkingStore#20462
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Nyholm commentedNov 9, 2016
I like this change. It makes it easier to understand the differences between the two. 👍 |
wouterj commentedNov 9, 2016
👍 Lot more descriptive (still a bit confused by "MarkingStore" but it's probably more correct if you're into the workflow nets) |
Nyholm commentedNov 9, 2016
The term is "marking" from the Petri net and that is something we should not change. But "store".. if we are not happy with it we can maybe could consider renaming it to "Strategy" |
lyrixx commentedNov 9, 2016
Actually, I made the So this is the minimal config: Workflow: framework:workflows:article:supports: -AppBundle\Entity\Articleplaces: -draft -wait_for_journalist -approved_by_journalist -wait_for_spellchecker -approved_by_spellchecker -publishedtransitions:request_review:from:draftto: -wait_for_journalist -wait_for_spellcheckerjournalist_approval:from:wait_for_journalistto:approved_by_journalistspellchecker_approval:from:wait_for_spellcheckerto:approved_by_spellcheckerpublish:from: -approved_by_journalist -approved_by_spellcheckerto:published state machine: task:type:state_machinesupports: -AppBundle\Entity\Taskplaces: -backlog -in_progress -donetransitions:processing:from:backlogto:in_progressdone:from:in_progressto:done As you can see, we never talk about the marking store. About the class name: it really save / get the Marking from a POPO. So I prefer MarkingStore to MarkingStrategy. |
Nyholm commentedNov 9, 2016
Im also fine with "store". I just tried to consider the alternatives because Wouter pointed it out. |
nicolas-grekas commentedNov 9, 2016
👍 |
| /** | ||
| *PropertyAccessorMarkingStore. | ||
| *MultipleStateMarkingStore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should describe the class instead of just using the class name, which does not really help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I updated the wording.
a1dc444 to0c1cba2Comparefabpot commentedNov 9, 2016
👍 |
* ScalarMarkingStore -> SingleStateMarkingStore* PropertyAccessorMarkingStore -> MultipleStateMarkingStoreAnd I also made optionnal the `marking_store` config, to let thecomponant choose the best marking store depending on the type(state_machine or workflow).
This PR was merged into the 3.2-dev branch.Discussion----------[Workfow] Rename current MarkingStore| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | yes (in un-released branch)| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -* ScalarMarkingStore -> SingleStateMarkingStore* PropertyAccessorMarkingStore -> MultipleStateMarkingStoreAnd I also made optionnal the `marking_store` config, to let thecomponant choose the best marking store depending on the type(state_machine or workflow).---Note: I did that to ease the on-boardingCommits-------08464c9 [Workfow] Rename current MarkingStore
stof commentedNov 9, 2016
PropertyAccessorMarkingStore -> MultipleStateMarkingStore looks weird to me. This marking store is special because it relies on PropertyAccess. We could have other marking stores supporting multiple state and using a different implementation internally |
Nyholm commentedNov 9, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
You are technically correct but in practice it will be considered an edge case that you would need a MultipleStateMarkingStore that has another implementation than PropertyAccess. Since it is rare, I'm fine with the name MultipleStateMarkingStore because it is way easier in to understand what it does. |
| * subject. | ||
| * | ||
| * This store deals with a "multiple state" Marking. It means a subject can be | ||
| * in many state at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
states
| /** | ||
| *PropertyAccessorMarkingStore constructor. | ||
| *MultipleStateMarkingStore constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't add any value.
| * SingleStateMarkingStore stores the marking into a property of the subject. | ||
| * | ||
| * This store deals with a "single state" Marking. It means a subject can be in | ||
| * one and only state at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[...] and only one state [...]
| /** | ||
| *ScalarMarkingStore constructor. | ||
| *SingleStateMarkingStore constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same as above
Uh oh!
There was an error while loading.Please reload this page.
And I also made optionnal the
marking_storeconfig, to let thecomponant choose the best marking store depending on the type
(state_machine or workflow).
Note: I did that to ease the on-boarding