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

WIP: Example plugin in Rust #11

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

WIP: Example plugin in Rust #11

wants to merge 7 commits into from

Conversation

heeckhau
Copy link
Member

No description provided.

@heeckhau heeckhau requested a review from sinui0 August 27, 2024 15:56
@heeckhau heeckhau marked this pull request as ready for review September 3, 2024 08:12
@heeckhau heeckhau requested a review from th4s September 18, 2024 07:43
Copy link
Member

@th4s th4s left a comment

Choose a reason for hiding this comment

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

Nice 🚀

Some comments, mostly about simplification.

examples/fitbit-rs/src/types.rs Show resolved Hide resolved
Comment on lines 21 to 25
if let Some(host_cookies) = cookies.get(hostname) {
Ok(host_cookies.clone())
} else {
Err(format!("Cannot find cookies for {}", hostname))
}
Copy link
Member

Choose a reason for hiding this comment

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

Here we can do

    cookies
        .get(hostname)
        .cloned()
        .ok_or_else(|| format!("Cannot find cookies for {}", hostname))

use extism_pdk::*;

pub fn get_cookies_by_host(hostname: &str) -> Result<HashMap<String, String>, String> {
let cookies_result = get("cookies");
Copy link
Member

Choose a reason for hiding this comment

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

If the function would return anyhow::Error instead of String like extism-pdk does, we can make use of ?

let cookies = get("cookies")?;

But even without it we can do

let cookies = get("cookies").map_err(|err| err.to_string())?;

Comment on lines 10 to 14
let cookies_json = match cookies_result {
Ok(Some(json_str)) => json_str,
Ok(None) => return Err("No cookies found in the configuration.".to_string()),
Err(e) => return Err(format!("Error retrieving cookies: {}", e)),
};
Copy link
Member

Choose a reason for hiding this comment

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

Then this becomes

    let cookies = cookies.ok_or_else(|| "No cookies found in the configuration.".to_string())?;

}

#[plugin_fn]
pub fn start() -> FnResult<Json<bool>> {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a bool it would be nicer to have a anyhow::Result<()>. Then you can also add error messages similarly to how you do in utils.rs.

}

#[plugin_fn]
pub fn two() -> FnResult<Json<Option<String>>> {
Copy link
Member

Choose a reason for hiding this comment

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

This function never returns an error, but logs it. Is this because of the plugin_fn constraints?

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