#491 Context manager for resetting fields #494
Conversation
Codecov Report
@@ Coverage Diff @@
## master #494 +/- ##
==========================================
+ Coverage 94.70% 95.07% +0.37%
==========================================
Files 6 6
Lines 774 812 +38
==========================================
+ Hits 733 772 +39
+ Misses 41 40 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is really brilliant @tumb1er !!! Sorry for having taken so long to make the review. I had given a first look the first day, but I wanted to read it a couple more time to be sure to get the whole picture.
I've added various suggestions, feel free to take the ones you like! :-)
IMHO this deserves a 5.0 release
| class FieldsContext: | ||
| def __init__(self, tracker, *fields, state=None): | ||
| if state is None: | ||
| state = {} |
There was a problem hiding this comment.
state may be passed as a parameter to FieldsContext.__init__, it will be not so obvious that there should be defaultdict instead of dict.
There was a problem hiding this comment.
You're right, but what is the real purpose of being able to do so, as the behaviour is very specific to a dict of ints? Is it really desirable that this gets passed as parameter?
There was a problem hiding this comment.
Nested contexts for same object should share same state (1), while contexts for another objects should have independent states (2). We can't use singleton because of (2), but need some shared storage for (1).
For (1) we can use single FieldsContext instance, implement a stack with field lists, but it looks too complicated both for FieldsContext implementation and helper methods that return FieldsContext. May be I'm wrong here and implementing re-entrance will be easier.
Could you think for some time about FIeldsContext fields stack and re-entrance? If it's good idea from your point of view, I'll refactor this at weekend.
There was a problem hiding this comment.
Hi @tumb1er I'm not really sure this will reduce the complexity, so I guess you'd better leave it as it is. Maybe you could also here add a docstring that explains the architecture.
There was a problem hiding this comment.
Complexity - bad, docstrings - good. I'll do it.
There was a problem hiding this comment.
is it possible to split this pr in smaller ones?
There was a problem hiding this comment.
In this PR:
- Two classes for new mechanics were introduced for fields state tracking
- Some helper methods added that simplify using new mechanics in new code
- default monkey-patched methods refactored to use new mechanics
I can split it, but each one will be more or less useless without another one.
There was a problem hiding this comment.
@auvipy it makes no sense to split this in smaller PRs! This is providing a new very flexible functionality to the FieldTracker
There was a problem hiding this comment.
Added docstrings for FieldsContext methods.
model_utils/tracker.py
Outdated
| ... # f1 is reset after outer context exit | ||
| >>> | ||
|
|
||
| * Note that fields are countedbe passing same state dict |
There was a problem hiding this comment.
I'm not really getting what you meant here, maybe rephrase/correct it?
There was a problem hiding this comment.
Now I'm not also :) I guess I meant "counted by".
I'll fix this after another discussion #494 (comment)
| class FieldsContext: | ||
| def __init__(self, tracker, *fields, state=None): | ||
| if state is None: | ||
| state = {} |
There was a problem hiding this comment.
is it possible to split this pr in smaller ones?
Breaking changes were introduced earlier, with 4.1.0 release. This PR changes implementation, introduces new functionality but preserves 4.1.0 behavior for existing code. |
|
LGTM. @auvipy up to you to check another time this MR, but IMHO it can be merged and is a great addition. |
|
yes let me revew |
Problem
FieldTrackerstate reset behavior described in Undocument breaking changes in FieldTracker in Version 4.1 #491Solution
This PR introduces
FieldsContextcontext manager which allows to postpone state reset till outermost context exitCommandments
CHANGES.rstfile to describe the changes, and quote according issue withGH-<issue_number>.Closes #491