alias __setattr__ to __setitem__ in Results#3281
alias __setattr__ to __setitem__ in Results#3281lilyminium merged 8 commits intoMDAnalysis:developfrom
Conversation
lilyminium
force-pushed
the
results-getattr
branch
from
52d0bb9 to
cb2c34e
Compare
Codecov Report
@@ Coverage Diff @@
## develop #3281 +/- ##
===========================================
- Coverage 93.03% 93.03% -0.01%
===========================================
Files 172 172
Lines 22732 22731 -1
Branches 3194 3192 -2
===========================================
- Hits 21149 21148 -1
Misses 1533 1533
Partials 50 50
Continue to review full report at Codecov.
|
PicoCentauri
left a comment
PicoCentauri
left a comment
There was a problem hiding this comment.
This is a smart PR!
| super().__init__(**kwargs) | ||
| self._dict_frozen = True | ||
| self.__dict__["data"] = {} | ||
| self.update(kwargs) |
There was a problem hiding this comment.
UserDict allow to initlize the instance with passing a dictionary. This does not work anymore. I suggest you copy the four lines
if dict is not None:
self.update(dict)
if kwargs:
self.update(kwargs)from cpython, adjust the arguments, add a test and everything is fine.
There was a problem hiding this comment.
Are you happy with my somewhat lazier solution? :)
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Looks good to me.
(I was debating if we should have a test showing that two instances do not interfere with each other but that seems un-necessary with the current implementation — it just makes use of the way that Python resolves class and instance attribute access.)
|
@PicoCentauri are you happy with the changes? @PicoCentauri @lilyminium please merge when ready. |
lilyminium
force-pushed
the
results-getattr
branch
from
619d937 to
5ed868a
Compare
|
Should I update CHANGELOG? The Results PR itself was very recent. |
|
I'd say no — normally we update CHANGELOG for anything that has an associated issue. But in this case this is a hotfix and the CHANGELOG entry would be more confusing than anything else. |
|
I'll leave this open for another few hours in case @cbouy or @PicoCentauri have comments :) |
cbouy
left a comment
cbouy
left a comment
There was a problem hiding this comment.
Sorry didn't get much time but all looks good!
lilyminium
changed the title
5 participants
Fixes #3282
Changes made in this Pull Request:
PR Checklist