impl std::error::Error for parameter errors

[alluring-mushroom]

Hello 😃

I have been testing this excellent project for work.
In using eyre for much of my error handling, I have found that the errors implemented in paramaters.rs do not implement std::error::Error, such as DeclarationError:

ros2_rust/rclrs/src/parameter.rs

Line 629 in e485b1c

pub enum DeclarationError {

Would you accept a PR containing these?

I assume you do not wish to depend on thiserror as you haven't used it for RclrsError. Is there any advice on the Display impls for any of these? I think many of the comments should suffice, such this one:

ros2_rust/rclrs/src/parameter.rs

Line 630 in e485b1c

/// Parameter was already declared and a new declaration was attempted.

Thank you for your time.

[jhdcs]

Hello!

Glad you are enjoying rclrs so far, and I hope it proves useful for your work! And thank you for the suggestion - yes, I think we probably would accept a PR for implementing std::error::Error.

I think that using the the comments would be a good start for the Display implementations for the errors. However, as you're doing so, think about whether that Display implementation would help you figure out what might be going wrong with your program. If you think what the comments say aren't enough to help steer an end user (such as yourself or your work colleagues), it may mean that we need to expand what's Displayed during an error.

If anything is unclear, or confusing, please feel free to reach out to us! We will do our best to help. And once again, thank you for the suggestion, and offering to help fix it! It means a lot to us!

[alluring-mushroom]

Hello, thank you for taking the time 😄

I have created a minimal pull request at #413, just using the comments with minor changes.

First, would you prefer discussion here, or on the PR itself?

Then, I have some questions.

---

1. It occured to me that good error messages that are displayed to an end user for parameters should likely include the parameter in question: its name and also possibly the description used in ParameterBuilder. This would require altering the Error struct to store the ParameterBuilder, or at least the relevant information. How do you feel about that?

   The alternative is to use the features of the error handling library, since these are intended for human consumption directly, It is likely that a user would be using eyre or anyhow like me. In this case, the user would use something like WrapErr to add context, changing something like this (eyre):
   

   to this:
   

   This is what I defaulted to doing, but it does lead to more boiler plate, as the most robust way to do this is to define the name and description in variables before each parameter declaration, and part of the reason the Error trait (and the ? operator) is to remove boilerplate, I feel. I am keen to hear your thoughts.

2. Do you think it is a good idea to include ros2 links in these messages? for the error in the previous discussion, DeclarationError::NoValueAvailable, a potentially improved format could be to refer to the ros2 documentation:

   parameter was declared as non-optional but no value was available. see https://docs.ros.org/en/rolling/Concepts/Basic/About-Parameters.html

3. I'm not sure about the change in rclrs/src/parameter/value.rs , as that code was fairly dense. I wasn't immediatly sure if RmwParameterConversionError would be user facing and would need an Error impl, originally I had only found the Errors in rclrs/src/parameter.rs

[jhdcs]

Discussions about a PR are usually best to do on the PR in question, so I think these questions would be better answered over there.

[luca-della-vedova]

This has been fixed by #413 and can be closed