Add missing throws documentation to rosbag_cpp writer open() #1788 #1789
Conversation
Document that open() may throw. Signed-off-by: Georg Dorndorf <[email protected]>
gDorndorf
requested review from
emersonknapp and
james-rms
and removed request for
a team
| * \throws runtime_error if | ||
| * database directory already exists, | ||
| * failed to create database directory, | ||
| * no storage could be initialized, | ||
| * invalid bag splitting size given, | ||
| * max cache size less or equal 0 when snapshot mode is enabled. |
There was a problem hiding this comment.
I am not sure if we want to list the all possible exception here, but at least indent is needed as below.
| * \throws runtime_error if | |
| * database directory already exists, | |
| * failed to create database directory, | |
| * no storage could be initialized, | |
| * invalid bag splitting size given, | |
| * max cache size less or equal 0 when snapshot mode is enabled. | |
| * \throws runtime_error if database directory already exists, | |
| * failed to create database directory, no storage could be initialized, | |
| * invalid bag splitting size given, | |
| * max cache size less or equal 0 when snapshot mode is enabled. |
There was a problem hiding this comment.
I agree that introduces some maintenance hazard keeping doc and implementation in sync, but it could be practical to have it there.
I'd also be fine with just a hint at some general throw behavior here.
So I'd leave it up to any reviewer as I am not too familiar with the style use throughout the rest of the codebase.
Remove documentation of implementation specific throw behaviour Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Georg Dorndorf <[email protected]>
Fix documentation intendation Co-authored-by: Tomoya Fujita <[email protected]> Signed-off-by: Georg Dorndorf <[email protected]>
There was a problem hiding this comment.
@gDorndorf Thanks for PR and attempt to make our Rosbag2 better.
However, in this particular case documenting about possible throwing exceptions is not trivial. Because this is an upper rosbag2_cpp layer API which is calling storage plugin API inside.
Each storage plugin may throw or not and what type of exceptions it throws and in what cases are implementation details for concrete storage plugin.
What we can do is make some notes that certain functions may throw if the underlying rosbag2_storage plugin API also throws.
Update: we can be more specific and refer to the concrete storage_plugin API interface.
|
Possibly a better API would be a no-throw |
Document that open() may throw.