DM-10777: Create TransformBoundedField #24
Conversation
include/astshim/Object.h
Outdated
| For two objects be equal, they both must have the same attributes and all contained objects | ||
| must be equal. However, this test ignores the difference between attributes that are not set | ||
| and attributes that are set to their default value, because that difference is irrelevant | ||
| to most behavior. |
There was a problem hiding this comment.
Most, but not all. Would be nice to know the difference. Perhaps it is important.
There was a problem hiding this comment.
You raise a good point. The AST manual is somewhat vague on this and I'm going to play it safe and not ignore the difference. That will require a tweak in afw's Transform constructor, but it's probably worth the tweak.
src/Object.cc
Outdated
| std::replace(thisStr.begin(), thisStr.end(), '#', ' '); | ||
| std::replace(rhsStr.begin(), rhsStr.end(), '#', ' '); | ||
|
|
||
| return rhsStr == thisStr; |
There was a problem hiding this comment.
Hmm, I can't say I'm wildly enthusiastic about comparing equality through comparing string representations. Is there really no better way?
There was a problem hiding this comment.
I agree, but unfortunately there is no better way.
| frame3 = astshim.Frame(2) | ||
| frame3.title = "Frame 3" | ||
| frameSet3 = astshim.FrameSet(frame3) | ||
| self.assertNotEqual(frameSet1, frameSet3) |
There was a problem hiding this comment.
You test for the complicated cases, but not the easy ones (where there is no difference between unset and default value). Say: frameSet1 == frameSet1 and astshim.Frame(2) == astshim.Frame(2). Unlikely to fail, but still.
There was a problem hiding this comment.
Additionally you may want a test for a more complicated case where multiple attributes are present.
There was a problem hiding this comment.
And what happens if say title = "bla#with this? E.g. one of the things in the printed representation has a hash? I don't know if that can happen, but it may be a problem with the string comparison.
There was a problem hiding this comment.
I added a test for two objects with equal attributes. Good suggestion!
I don't think testing frameSet1 against itself is interesting because the fallback is pointer equality.
The attribute containing a hash is a good point, but no longer relevant because I am no longer stripping hash marks.
tests/test_object.py
Outdated
| show2 = obj.show(False).strip() | ||
| self.assertTrue(show2.startswith("Begin ZoomMap")) | ||
| self.assertNotIn("# Zoom", show2) | ||
|
|
There was a problem hiding this comment.
What if it does something else? Say insert #zoom or otherwise change the expected test? Isn't it better to explicitly check the expected string for equality?
There was a problem hiding this comment.
Fair enough. I'll test for exact strings
Equality is based on the string representation because AST offers no better way.
No description provided.