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-13084: Be smarter about combining metadata from FITS headers #32
Conversation
and the same test for PropertyList.copy
9d292a1
to
6fd1bf0
Compare
auto vp = std::make_shared<vector<boost::any>>(*(sj->second)); | ||
_set(dest, vp); | ||
} | ||
} |
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.
There is a small bit of code duplication here: _set(det, vp)
. I thought about eliminating it by initializing vp
to a blank vector outside of the if/else, appending to it in the asScalar
branch and replacing it in the else
branch. I felt the current code might be a bit easier to read, but will gladly change it on request.
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 that's fine.
include/lsst/daf/base/PropertyList.h
Outdated
virtual ~PropertyList(void); | ||
|
||
// Accessors | ||
/** | ||
* Make a deep copy the PropertyList and all of its contents. |
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.
"copy of the"
include/lsst/daf/base/PropertySet.h
Outdated
// Accessors | ||
|
||
/** | ||
* Make a deep copy the PropertySet and all of its contents. |
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.
"copy of the"
include/lsst/daf/base/PropertySet.h
Outdated
virtual void combine(ConstPtr source); | ||
// All vectors from the source are add()ed to the destination with the | ||
// same names. Types must match. |
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.
Should this ("types must match") move up to the doxygen comment instead of being removed? Or is the @throws TypeError
sufficient?
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'll add a note to the doc.
include/lsst/daf/base/PropertySet.h
Outdated
AnyMap::iterator _find(std::string const& name); | ||
|
||
/* Find2 the property name (possibly hierarchical). Const version. |
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.
"Find2"?
auto vp = std::make_shared<vector<boost::any>>(*(sj->second)); | ||
_set(dest, vp); | ||
} | ||
} |
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 that's fine.
And remove @file directives, which we now prefer not to have
and PropertyList::copy.
6fd1bf0
to
c2aa5ea
Compare
No description provided.