Add BaseNode.equals #14
Conversation
Pike
left a comment
Pike
left a comment
There was a problem hiding this comment.
This looks pretty good, just two nits so far.
fluent/syntax/ast.py
Outdated
| self_fields = [getattr(self, key) for key in keys] | ||
| other_fields = [getattr(other, key) for key in keys] | ||
|
|
||
| for key, field1, field2 in izip(keys, self_fields, other_fields): |
There was a problem hiding this comment.
I'd just iterate over keys, unsorted, and assign field1 and field2 inside the loop
fluent/syntax/ast.py
Outdated
| 'variants': lambda elem: elem.key.name, | ||
| } | ||
|
|
||
| if key in field_sorting.keys(): |
There was a problem hiding this comment.
Just
if key in field_sorting:
Pike
left a comment
Pike
left a comment
There was a problem hiding this comment.
This looks good to me. Just a few comments about sugar and icing on the tests.
| } | ||
| """) | ||
|
|
||
| self.assertTrue(message1.equals(message1)) |
There was a problem hiding this comment.
Does it make sense to test for equality against the identity traversal, too?
If so, add that to the tests in this class?
tests/syntax/test_equals.py
Outdated
| self.assertTrue(message2.equals(message1)) | ||
|
|
||
|
|
||
| class TestSpansEqual(unittest.TestCase): |
There was a problem hiding this comment.
I guess the name here is older than your inequality test? Rename this to TestWithSpans or so?
tests/syntax/test_equals.py
Outdated
| for a, b in messages: | ||
| self.assertTrue(a.equals(b, with_spans=True)) | ||
|
|
||
| def test_differ_with_spans(self): |
There was a problem hiding this comment.
Move this test to the end? Then the four combos of flags to Parser and .equals are all in a row?
2 participants
No description provided.