[MERGE] Remove most calls to safe_file_id and safe_revision_id.
Ian Clatworthy
ian.clatworthy at internode.on.net
Wed Sep 26 06:49:53 BST 2007
Martin Pool wrote:
> As previously suggested by John, this removes the compatibility
> interface that accepted unicode objects for revision_id parameters.
The first question that came to mind in this review as "OK, what's
left?". Outside the definition and tests, I found 3 uses of
safe_revision_id left in the code:
* builtins.py
* workingtree.py
* bundle/bundle_data.py
All of those had the warn=False parameter (indicating user input) so
that looks good to me. Likewise, I found 3 uses of safe_file_id and all
of those had warn=False as well. Checking the code, it looks like
leaving them there is correct as well.
In summary, I don't believe you've missed anything.
bb:tweak
> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py 2007-09-13 22:41:38 +0000
> +++ bzrlib/workingtree.py 2007-09-25 08:14:12 +0000
> - # TODO: jam 20070209 This should be redundant, as the revision_id
> - # as all callers should have already converted the revision_id to
> - # utf8
> - inventory.revision_id = osutils.safe_revision_id(revision_id)
> return xml7.serializer_v7.write_inventory_to_string(inventory)
This is a semantic change in that inventory.revision_id is no longer
being set here. Was that deliberate? (It stands out as being more than a
mechanical change as 99% of the other changes in this patch are.) I
haven't analysed the various data flows but it doesn't look a safe
change.
> @@ -1975,15 +1951,6 @@
> @needs_tree_write_lock
> def set_root_id(self, file_id):
> """Set the root id for this tree."""
> - # for compatability
> - if file_id is None:
> - symbol_versioning.warn(symbol_versioning.zero_twelve
> - % 'WorkingTree.set_root_id with fileid=None',
> - DeprecationWarning,
> - stacklevel=3)
> - file_id = ROOT_ID
> - else:
> - file_id = osutils.safe_file_id(file_id)
> self._set_root_id(file_id)
This change breaks the test suite:
File
"/home/ian/bzr/bzr.tmp/bzrlib/tests/workingtree_implementations/test_set_root_id.py",
line 33, in test_set_None
tree.set_root_id, None)
AssertionError: not equal:
a = ['WorkingTree.set_root_id with fileid=None was deprecated in version
0.12.']
b = []
It wouldn't hurt to leave the if clause there. Otherwise, the test suite
will need a tweak if you'd prefer (0.12 was a while ago now).
Ian C.
More information about the bazaar
mailing list