100% coverage #12
100% coverage #12
Conversation
|
#11 was merged, rebased this on master. |
| return bool(self.prinper or self.prinrole or self.permrole) | ||
|
|
||
| __nonzero__ = __bool__ |
There was a problem hiding this comment.
TBH I think this is a Python 3 bugfix that deserves its own line in the changelog.
There was a problem hiding this comment.
There is one:
- Make
SecurityMapandAnnotationGrantInfohave proper truth
behaviour on Python 3; previously they were always true.
| @@ -146,7 +148,7 @@ def _changed(self): | |||
| if isinstance(map, PersistentSecurityMap): | |||
| map._p_changed = 1 | |||
| else: | |||
| map = PersistentSecurityMap() | |||
| map = self.map = PersistentSecurityMap() | |||
There was a problem hiding this comment.
This also looks like a bugfix that could use a changelog entry?
There was a problem hiding this comment.
I viewed it as purely an internal performance fix, which only comes into play the first transaction this object is used in (i.e., rare). But I can add a changelog; if I do that I need to add a test that verifies this particular scenario.
| return prinrole.items() | ||
| return () | ||
| prinrole = self.prinrole.get(principal, {}) | ||
| return list(prinrole.items()) |
There was a problem hiding this comment.
I have some worries about performance implications of these changes. Perhaps we could only invoke list() when prinrole.items() doesn't return one already, i.e. only on Python 3?
There was a problem hiding this comment.
I'm not sure that's really a problem because (1) The only references to either IGrantInfo (the only interface this class implements, which does not extend anything except Interface) or AnnotationGrantInfo across both zopefoundation and plone organizations are in this repository; and (2) it was completely untested before this. So I don't think it's really used.
Plus, Python 2 is going away so the number of affected users is small and getting smaller 😄
But if you feel it's important I will happily make a change.
There was a problem hiding this comment.
Nah, your arguments sound very convincing.
|
Thanks! Would you like to make a PyPI release? I don't have access. |
|
@jamadden Thank you for tackling the issue I have created. I added you as owner of this package on PyPI. |
|
Thanks! I've pushed 4.3.0 to PyPI. |
|
@jamadden Thank you, too. |
Fixes #10
Currently based on #11 so that CI will run.