Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#491 Context manager for resetting fields #494
#491 Context manager for resetting fields #494
Changes from 11 commits
66525d7
a661ac9
9c215ab
7ce84d3
36bdb38
920db5e
0eb5545
d7df35c
7d95317
59ab645
eb8df82
3249a5c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not really getting what you meant here, maybe rephrase/correct it?
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.
Now I'm not also :) I guess I meant "counted by".
I'll fix this after another discussion #494 (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.
Fixed typo here.
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.
Or maybe a
defaultdict(0)
?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.
state
may be passed as a parameter toFieldsContext.__init__
, it will be not so obvious that there should bedefaultdict
instead ofdict
.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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complexity - bad, docstrings - good. I'll do it.
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.
is it possible to split this pr in smaller ones?
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.
In this PR:
I can split it, but each one will be more or less useless without another one.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added docstrings for FieldsContext methods.