Skip to content

added example to RclrsError #508

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

Closed
wants to merge 1 commit into from

Conversation

Guelakais
Copy link
Contributor

No description provided.

@Guelakais Guelakais force-pushed the ErrorMsgs_example_usage branch from b75803a to 5ba2dad Compare August 14, 2025 12:39
Copy link
Collaborator

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's a no for me, the example really shows how to build a variant and write a match statement

@Guelakais
Copy link
Contributor Author

Sorry it's a no for me, the example really shows how to build a variant and write a match statement

Then you haven't really looked at it. The match statement is merely a means to an end, ensuring that the evaluation works properly. The point is that the error is created correctly and can be evaluated using an assert_eq call. In this case, a match works quite well for this purpose.

@luca-della-vedova
Copy link
Collaborator

Let me paraphrase it for Option.

let val: Option<u32> = Some(123);
match val {
    Some(v) => assert_eq!(v, 123),
    None => panic!();
}

Would you say this is a useful example?

@Guelakais
Copy link
Contributor Author

Guelakais commented Aug 15, 2025

according to the documentation of an Option from the standard library, yes:

let mut x = Some(2);
match x.as_mut() {
    Some(v) => *v = 42,
    None => {},
}
assert_eq!(x, Some(42));

Do you want to argue now against the standard library of rust?

@luca-della-vedova
Copy link
Collaborator

Do you want to argue now against the standard library of rust?

I suppose we have different interpretations, agree to disagree

@mxgrey
Copy link
Collaborator

mxgrey commented Aug 15, 2025

@Guelakais Many of the PRs that you open contain far more noise than substance. In my view, this PR is strong example of noise. At best this example is teaching the reader the syntax of a match statement, which is not a good use of a docstring for this library. If every docstring of every library was needlessly demonstrating language syntax, it would become much harder for users to find the information that they actually need from each library's documentation.

It's important that you understand, @Guelakais, that no one is entitled to have their contributions accepted to an open source project. Every time you open a PR, you are subjecting your work to public criticism, including criticism that you don't agree with. I would urge you to think carefully about whether each PR you open or each comment you leave on an open source project has substantial value. Are you creating something that will actually help people? Are you moving a conversation forward in a constructive way? If the answer to either of those questions is not a strong yes, then it's likely that you are actually wasting the time of the people who are managing that project. And their time is a precious resource that none of us in the community want to see wasted.

Don't be surprised if the kind of behavior that you're demonstrating eventually gets you banned from participating in an open source project for violating the code of conduct.

@Guelakais
Copy link
Contributor Author

I don't really understand what your problem is. Do you have something against documentation or against the match statement? You can also formulate this without the match statement. Then it would look like this:

use rclrs::{RclReturnCode,RclrsError};
 let error: RclrsError = RclrsError::RclError { code: RclReturnCode::Timeout, msg: None };
 if let RclrsError::RclError { msg, .. } = &error {
     assert_eq!(msg, &None);
}

@mxgrey
Copy link
Collaborator

mxgrey commented Aug 15, 2025

Standard library documentation, especially for a module as fundamental as Option, is largely targeting brand new users who are likely to need an introduction to the basics of the language.

Users of rclrs are not expected to be learning the Rust language from the documentation of rclrs. There are much better resources for that, such as the documentation of the standard library.

@Guelakais Guelakais force-pushed the ErrorMsgs_example_usage branch from 5ba2dad to 31a5fd2 Compare August 15, 2025 07:12
@esteve
Copy link
Collaborator

esteve commented Aug 15, 2025

@Guelakais nobody is arguing against the standard library or the match keyword, but that the "example" you've added has no value. I agree with @mxgrey and @luca-della-vedova

@esteve esteve closed this Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants