-
Notifications
You must be signed in to change notification settings - Fork 106
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
Use same initialization order in Scenario as Collect #780
Conversation
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 is a really great time to make these kinds of changes since we're just exposing Scenario for the first time as an intentional public interface and we are planning a major version bump 👍
@@ -1629,11 +1629,20 @@ public Extras<Externalizable> getExtras() { | |||
return extras; | |||
} | |||
|
|||
/** | |||
* @deprecated use {@link FormEntryController#addFilterStrategy(FilterStrategy)} instead |
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.
Since we're doing a major version bump anyway, what do you think about removing these right away? It's very unlikely there are existing consumers and if there are it's an easy change. We'd just want to make sure to track these breaking changes somewhere so we can document them.
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'd thought about that, but we'd need to do significant work to move EvaluationContext
ownership to FormEntryController
(as that's what these methods affect). I gave doing a minimal change (where we still end up with a coupling between FormDef
and FormEntryController
but the EvaluationContext
moves) a go, but it was pretty involved.
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.
Ah, ok, I thought it was an easier change that that. Sounds good.
This switches the initialization order in
Scenario
to match Collect (FormDef#initialize
is called after creating theFormEntryController
). It also makes it possible to pass in a custom function for constructing theFormEntryController
to allow tests to use plugins (like#addFilterStrategy
).