-
Notifications
You must be signed in to change notification settings - Fork 107
Add override_state_values support to initialize_from #632
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
base: main
Are you sure you want to change the base?
Conversation
|
@Smitaambiger cool. Can you add a unit test for this please? |
|
@skrawcz |
|
@skrawcz Please let me know if you’d like the test structured differently or placed elsewhere. Happy to update! |
tests/core/test_application.py
Outdated
| from burr.core.application import ApplicationBuilder | ||
| from burr.core.state import State | ||
| from burr.core.persistence import BaseStateLoader |
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.
can you remove these imports -- there should be no need for them. If they are missing add them to the top please.
burr/core/application.py
Outdated
| # there was something | ||
| last_position = load_result["position"] | ||
| self.state = load_result["state"] | ||
| if getattr(self, "override_state_values", None): |
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 overly defensive. Logically this should always resolve given this change.
| if getattr(self, "override_state_values", None): | |
| if self.override_state_values: |
skrawcz
left a comment
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.
Thanks. Some minor clean up please.
|
@Smitaambiger your test also fails:
|
…mbiger/burr into override-state-initialize
|
@skrawcz Thanks for the review! I’ve addressed the requested cleanup and test failure:
I verified the change locally with: pytest tests/core/test_application.py -k override_state_values
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======== 1 passed, 124 deselected, 2 warnings in 0.60s ========= |
@skrawcz Related to #416
This PR adds an optional
override_state_valuesparameter toApplicationBuilder.initialize_from, allowing callers to override or augment state values when initializing from a persisted state.Changes
override_state_valuesparameter toinitialize_fromHow I tested this
Notes
This change is backward-compatible and only applies when
override_state_valuesis explicitly passed.