<div>On 4/12/07, John Arbash Meinel <<a href="mailto:john@arbash-meinel.com">john@arbash-meinel.com</a>> wrote:<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> John Arbash Meinel has voted +0. Status is now: Waiting Comment: This has come quite a ways, and I would really like to see it in 0.16.</blockquote><div> me too </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> If you need some help, just ask, and I can try to do some of the things I mention here</blockquote><div> I'll try my best to get it done..  </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> I think this should have a NOTES WHEN UPGRADING: section in NEWS. Because it means that 'bzr rm' will change how it works. I know there have been mention of people who are already using 'bzr rm' and expecting it to leave the file alone (James Troup mentioned this for versioning files in /etc). So we need to make sure to convey this change well. (We aren't destroying the data, you can 'bzr revert' or 'bzr rm --keep', but people need to know about it).</blockquote><div> Should I just put the whole thing in a the NOTES WHEN UPGRADING section ? </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> I don't quite understand this change: -    only 'added' files will be removed.  If you specify both, then new files -    in the specified directories will be removed.  If the directories are +    only 'added' files will be removed.  If you specify both, then new +    files in the specified directories will be removed.  If the directories are It seems you re-wrapped the line, only to cause the next line to become *too* long.</blockquote><div> [DONE]  </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">I'm not sure about making these take multiple paths:       def failUnlessExists(self, path): -        """Fail unless path, which may be abs or relative, exists.""" -        self.failUnless(osutils.lexists(path),path+" does not exist") +        """Fail unless path or paths, which may be abs or relative, exist.""" +        if not isinstance(path, basestring): +            for p in path: +                self.failUnlessExists(p) +        else: +            self.failUnless(osutils.lexists(path),path+" does not exist") I think we generally avoid apis that use 'isinstance'. I'm probably about 75% okay on this, though. Since it is meant as a way to make writing tests easier, and as long as the tests are easier to write and easier to understand, it seems good.</blockquote><div> I'm leaving as is, unless someone is less than 50% ok with it.. :) </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> I prefer 'self.run_bzr' to 'self.runbzr', but that is more of a guideline than a requirement.</blockquote><div> ok I'll convert to run_bzr_captured, but 'self.runbzr' seemed more suited to lazy people like me... [DONE] * these tests follow this pattern:         (out, err) = self.run_bzr_captured(['remove', '--keep', 'a'])         self.assertEquals(out.strip(), "")         self.assertEquals (err.strip(), "removed a") which we might want to do better:         self.run_bzr_captured(['remove', '--keep', 'a'], out_regex='', err_regex="removed a") also can't I make it like runbzr, to auto expand a sting?   (your probably gona say no as you don't like isinstance magic :( </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> PEP8 says that: (out,err) = self.runbzr( should be (out, err) = self.</blockquote><div> [DONE] </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> You added a test class to test_workingtree.py, but it should probably be a test in workingtree_implementations/test_remove.py + +class TestRemove(TestCaseWithTransport): +    """Tests WorkingTree.remove """ It needs to be written slightly differently, first by inheriting from TestCaseWithWorkingTree. You can look at other 'workingtree_implementations' tests to get a reference for how to do it. Putting it in bzrlib/tests/test_workingtree.py means it will be tested with our current default WT format. Putting it in workingtree_implementations means it will get tested against all of our WT implementations (currently WT2, WT3, WT4). The biggest benefit is that it adds tests for any new implementations, to make sure we maintain interface consistency.</blockquote><div> [DONE]. all tests passed first time!  </div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> I don't see why you needed to do this: -                if not tree.has_id(file_id): +                if not tree.has_id(file_id) or not file_id in tree.inventory: If you found a case where "tree.has_id (file_id)" was returning True, but file_id not in tree.inventory, then you found a bug. And it should be fixed at the 'has_id()' layer.</blockquote><div> ...mm thats weird. I had to do this to make my tests pass, but I undid it now, and it works fine.. I think at the time I did it, I used self.changes_from(..., specific_files=...), wrong, since I didn't dereference them... [REMOVED]  </div></div>... I'm still busy with the rest, but will get it done asap.