Skip to content
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

web: v1: rest: mavlink: Add helper endpoint #104

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

patrickelectric
Copy link
Member

image

Copy link
Member

@joaoantoniocardoso joaoantoniocardoso left a comment

Choose a reason for hiding this comment

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

I didn't get the idea behind this. Why are we adding an endpoint to get the default message?

Comment on lines 3 to 6
use crate::{
drivers::rest::parse_query,
mavlink_json::{MAVLinkJSON, MAVLinkJSONHeader},
};
Copy link
Member

@joaoantoniocardoso joaoantoniocardoso Dec 12, 2024

Choose a reason for hiding this comment

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

We are trying to keep the imports tidy by putting the crate:: imports after the 3rd party, separated by a blank line, as in:

use std::sync::Arc;

use anyhow::Result;

use crate::callbacks::{Callbacks, MessageCallback};

inner: Default::default(),
message_id: Some(mavlink::Message::message_id(&message)),
};
let json = serde_json::to_string_pretty(&MAVLinkJSON { header, message }).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let json = serde_json::to_string_pretty(&MAVLinkJSON { header, message }).unwrap();
let json = serde_json::to_string_pretty(&MAVLinkJSON { header, message })?;

Copy link
Member Author

Choose a reason for hiding this comment

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

this operation will never fail.

Copy link
Member

Choose a reason for hiding this comment

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

then:

Suggested change
let json = serde_json::to_string_pretty(&MAVLinkJSON { header, message }).unwrap();
let json = serde_json::to_string_pretty(&MAVLinkJSON { header, message }).expect("Should never fail here");

Copy link
Member

Choose a reason for hiding this comment

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

Also, even if something never fails, it's still okay to use ? if we can

Copy link
Member Author

@patrickelectric patrickelectric Dec 12, 2024

Choose a reason for hiding this comment

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

But this will generate an invalid HTTP response that's not a json. Does not make more sense to add verbose code or a possible early return to a clear impossible operation. unwrap is there for a reason.

Copy link
Member

Choose a reason for hiding this comment

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

But this will generate an invalid HTTP response that's not a json. Does not make more sense to add verbose code or a possible early return to a clear impossible operation. unwrap is there for a reason.

right, just use expect, then, so I won't get confused when reading the code in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a trivial operation, the type is known in compile time.

([(header::CONTENT_TYPE, "application/json")], json).into_response()
}
Err(error) => {
let error_json = serde_json::to_string_pretty(&error).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let error_json = serde_json::to_string_pretty(&error).unwrap();
let error_json = serde_json::to_string_pretty(&error)?;

Copy link
Member Author

Choose a reason for hiding this comment

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

This operation should never fail.

@patrickelectric
Copy link
Member Author

patrickelectric commented Dec 12, 2024

I didn't get the idea behind this. Why are we adding an endpoint to get the default message?

This is a helper function, as exist in mavlink2rest for the user to use with the REST API, people don't have a serde serializer on their heads.

Copy link
Member

@joaoantoniocardoso joaoantoniocardoso left a comment

Choose a reason for hiding this comment

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

2 more, 2 less.. later I'll clean up the unwraps to make the code easier to follow.

From what I can remember, there are many unwraps around, and differently from these two, some of those were not infallible.

@joaoantoniocardoso joaoantoniocardoso merged commit c4c7656 into bluerobotics:master Dec 12, 2024
8 checks passed
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.

2 participants