Fix for string formatting error in Repository.py #6
Conversation
|
@mauritsvanrees Do you know enough of the code of this project to make a review of this PR? If not who else could? |
|
Looking at the contributors I have done a fair number of commits, but they are all packaging related. The first commit seems obviously fine, and it matches the title of the PR. |
There was a problem hiding this comment.
I do not feel competent enough to fully eveluate this PR:
- why this is needed?
- which issue it is going to fix?
- will there will be side effects?
Maybe adding a test will help to clarify things!
All in all it looks sane and seems a nice improvement to me.
I added some suggestions about code style but they can definitely be ignored.
| @@ -76,7 +76,7 @@ def replaceState(self, obj, new_state): | |||
| if obj.__class__ is not new_state.__class__: | |||
| raise VersionControlError( | |||
| "The class of the versioned object has changed. %s != %s" | |||
| % (repr(obj.__class__, new_state.__class__))) | |||
| % (repr(obj.__class__), repr(new_state.__class__))) | |||
There was a problem hiding this comment.
IMHO better:
"The class of the versioned object has changed. %r != %r" % (obj.__class__, new_state.__class__)but the current version is good as well
| info = getattr(object, '__vc_info__', None) | ||
| if info is None: | ||
| return False | ||
| return info.history_id in self._histories and \ |
There was a problem hiding this comment.
IMHO better and cleaner:
history = self._histories.get(info.history_id)
return history and history.hasVersionId(info.version_id)but the current version is good as well
| return {'contents': contents, 'attributes': attributes} | ||
| order = [] | ||
| if getattr(self.obj, '_objects', False): | ||
| order = [x['id'] for x in self.obj._objects] |
There was a problem hiding this comment.
IMHO better and more coincise:
order = [obj["id"] for obj in getattr(self.obj, '_objects', [])] but the current version works as well
| @@ -163,3 +166,18 @@ def restoreNonVersionedData(self, data): | |||
| # a BTreeFolder2, which doesn't need or want the | |||
| # _objects attribute. | |||
| # XXX This is a hackish way to check for BTreeFolder2s. | |||
| # Yes, we're repeating ourselves: | |||
| if not hasattr(obj, '_tree'): | |||
There was a problem hiding this comment.
I am missing context and knowledge why this needs to be done if we have no _tree :)
There was a problem hiding this comment.
That seems explained in the existing comment above it: when there is no _tree attribute, the object is a BTreeFolder which does not care about an _objects attribute, so we don't need to do anything.
I don't know enough to know if this is true, but I trust the original comment.
I opened the PR after the first commit, and the subsequent commits got added automatically to it, not really sure why. Anyway, the next two commits fix other issues: Commit 1) When items are imported by ZEXP from another site and have Commit 2) When an ordered folder item is cloned, the order information is lost without this fix. |
If you make a PR from a branch, and then you push more commits to the branch, then the PR is automatically updated. That is why they got added. They could have been done in different pull requests, that could have speeded up merging one or more of them, because now a reviewer needs to understand the whole PR.
Thanks for the explanation, that makes it clearer. Looks good to me. I will merge. Thank you! |
Trivial fix for a longstanding string format bug.