On 12/07/11 19:39, Aaron Bentley wrote:
> Review: Disapprove
> This change is at the wrong level. A TreeTransform whose result lacks a tree root is valid, because it can be used to produce an empty PreviewTree.
>
> Instead, it should be done in TreeTransform.apply. That code already has functionality to ensure the root directory is not deleted, which makes your "if self.final_kind(self.root) is None: self.cancel_deletion(self.root)" redundant.
This already landed before your review, but I don't really understand
how TreeTransform.apply is better than TreeTransform.fix_tree_roots.
This particular code *is* necessary because without it the loom test
suite breaks (as discussed in Dublin). What alternative do you suggest
exactly?
Hi Aaron,
On 12/07/11 19:39, Aaron Bentley wrote: apply. That code already has functionality to ensure the root directory is not deleted, which makes your "if self.final_ kind(self. root) is None: self.cancel_ deletion( self.root) " redundant. fix_tree_ roots.
> Review: Disapprove
> This change is at the wrong level. A TreeTransform whose result lacks a tree root is valid, because it can be used to produce an empty PreviewTree.
>
> Instead, it should be done in TreeTransform.
This already landed before your review, but I don't really understand
how TreeTransform.apply is better than TreeTransform.
This particular code *is* necessary because without it the loom test
suite breaks (as discussed in Dublin). What alternative do you suggest
exactly?
Cheers,
Jelmer