add package resource related utility functions to cpp API #27
Conversation
these already existed in Python, see: #24
wjwwood
added
enhancement
|
| { | ||
| public: | ||
| AMENT_INDEX_CPP_PUBLIC | ||
| explicit PackageNotFoundError(const std::string & package_name); |
There was a problem hiding this comment.
Should the package name be stored as a member in the custom exception class?
| AMENT_INDEX_CPP_PUBLIC | ||
| virtual ~PackageNotFoundError(); | ||
|
|
||
| const std::string package_name; |
There was a problem hiding this comment.
Why should the member be const? For a non-const instance of this class the user might want to change this member.
There was a problem hiding this comment.
Because the package name is baked into the error message, and if you change this then it would be inconsistent with the what(), so you should only set it from the constructor. If you want a different package name in the error, I'd say create another object.
There was a problem hiding this comment.
I still don't think that the class should prescribe the member to be const.
There was a problem hiding this comment.
Unfortunately I don't agree. I've given a reason why it would be undesirable to change the package name after creation, but I haven't seen a reason why you would want to change it. I'm willing to change it if a convincing reason was given and that outweighs the downside I've already stated.
There was a problem hiding this comment.
The package for which the exception was thrown does not seem like it should be modifiable after the fact or in the traceback/error handling thus keeping it const would make sense. And relaxing the const constraint in the future will always be easier than adding it if there is a use case that needs it than the reverse.
There was a problem hiding this comment.
Btw, I looked at making a setter and getter for the package_name, as a compromise, but that would require me to override the what() method (storage for what() is not exposed in the exception class) and provide my own storage which would be redundant. Also the what() function is noexcept:
http://en.cppreference.com/w/cpp/error/exception/what
So, the creating of the new std::string storage would need to be done in the setter, more or less what the constructor is doing. So I don't think it would be any easier (more efficient) than creating a new exception if you needed to change the package name (still don't know why you would need to do so).
|
New CI for the current state:
|
These already existed in Python, see: #24