New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-21333: Implement afw.image.Filter replacement(s) #543
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, few comments/questions. I'm not very familiar with afw persistency mechanism but I see that tests cover everything.
// A separate boolean leads to easier implementations (at the cost of more | ||
// memory) than a unique_ptr<string>. | ||
// _band and _physical are part of the object state iff _hasBand and _hasPhysical, respectively | ||
bool _hasBand, _hasPhysical; | ||
std::string _band, _physical; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my education - I presume that label cannot be empty for actual physical/band, would it simplify things if you use empty string instead of separate bool flag for each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I can't think of a legitimate use for an actual empty string as either name, I distrust magic values. This approach leads to less surprising behavior, I think.
|
||
#ifndef DOXYGEN | ||
class FilterLabel; | ||
namespace impl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think people usually use detail
namespace for implementation-level global names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The developer guide said to use impl
in this case.
This class has factory methods and getters, but no other functionality.
Though in principle these could be tested separately, doing so would greatly increase the proportion of tests that use factories instead of keyword for construction, and I want to encourage the latter as the natural Python interface.
Since FilterLabel is a value-like object, all four methods are appropriate. Hashing is non-trivial because some members are part of the object state only some of the time; I've added a C++ unit test to check this.
547cbe1
to
2ee1984
Compare
This PR implements the
FilterLabel
class and unit-tests its functionality. Integration withExposure
is not yet supported, and will be added later.