Code review comment for lp:~spiv/bzr/hardlink-2a-408193

Revision history for this message
John A Meinel (jameinel) wrote :

46 + self.assertEqual(source_stat, target_stat)

^- It is generally better to use "self.assertEqualStat()".

I realize it isn't strictly required here, since you aren't doing an fstat versus lstat, etc.

118 + os.mkdir(self.test_home_dir + '/.bazaar')
119 + rules_filename = self.test_home_dir + '/.bazaar/rules'
120 + f = open(rules_filename, 'w')
121 + f.write('[name %s]\nrot13=yes\n' % (pattern,))
122 + f.close()

^- Could you use ".build_tree()" and ".build_tree_contents()" here? I'll also note that you should at least use 'wb', since I don't think you want to get \r\n content on Windows. (maybe this doesn't matter as much since it is a config file?)

Would it be easier to use something like "fn = config.get_rules_filename()" ? I don't know how that path is currently determined, but it doesn't seem like you should be going via self.test_home_dir. (If there isn't an api for it, just submit a bug, and leave the test alone.)

Otherwise I think the change is reasonable.

review: Approve

« Back to merge proposal