tree-building : implement exercise#796
tree-building : implement exercise#796ilya-khadykin merged 4 commits intoexercism:masterfrom clapmyhands:implement/tree-building
Conversation
ilya-khadykin
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Looks good to me
|
|
||
| def validateRecord(record): | ||
| if record.equal_id() and record.record_id != 0: | ||
| raise ValueError |
There was a problem hiding this comment.
Should probably provide the cause of the exception
| if record.equal_id() and record.record_id != 0: | ||
| raise ValueError | ||
| elif (not record.equal_id()) and record.parent_id >= record.record_id: | ||
| raise ValueError |
exercises/tree-building/example.py
Outdated
| def validateRecord(record): | ||
| if record.equal_id() and record.record_id != 0: | ||
| raise ValueError | ||
| elif (not record.equal_id()) and record.parent_id >= record.record_id: |
There was a problem hiding this comment.
redundant parentheses
|
@m-a-ge I've change it according to the review. Please check again and let me know if there's anything else to change 😄 . |
|
|
||
| def assert_node_is_branch(self, node, node_id, children_count): | ||
| self.assertEqual(node.node_id, node_id) | ||
| self.assertFalse(len(node.children) == 0) |
There was a problem hiding this comment.
assertNotEqual(len(node.children), 0) would be clearer.
|
|
||
| def assert_node_is_leaf(self, node, node_id): | ||
| self.assertEqual(node.node_id, node_id) | ||
| self.assertTrue(len(node.children) == 0) |
There was a problem hiding this comment.
assertEqual(len(node.children), 0) would be clearer.
|
@cmccandless I've changed it according to your review. Also I've been thinking, do I need to put some reason on the test where a |
|
@clapmyhands If I understand you correctly, do you mean something like this? with self.assertRaisesRegexp(ValueError, 'No root node'):
BuildTree(records)If that's what you mean, that would be a good idea 👍 |
|
@cmccandless I don't think it's possible. At least in my current code it can't since the checking of 'no root node' is the same as checking of 'non continuous' reason being: raise ValueErrorcan cover multiple failing cases. I was thinking more in line of comments like so: # no root node
with self.assertRaises(ValueError):
BuildTree(records)the name of the test might already be self-explanatory but for some tests such as def test_cycle_directly(self):it might be good to have further explanation on why that gives an error. p.s: what do you think about changing self.assertIs(root, None)to self.assertIsNone(root) |
|
@clapmyhands I agree that a few changes would be required to provide accurate exception details, but as a fellow programmer I would caution you against using the word "impossible" lightly 😉. At this time, this track does not have a requirement for exceptions to contain details, so I will leave it up to you whether to:
OR
Whichever you decide, I believe that should be enough to merge. |
|
@cmccandless I've changed it to use |
|
@clapmyhands I've left a couple comments. Just some consistency stuff. |
|
@cmccandless I've added comment for consistency. |
|
Thanks for working on this, @clapmyhands! |
|
@m-a-ge @cmccandless Thanks for being patient with me through it all. New contributor and hopefully a lasting one 😄 |
resolves #741