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

chore: upgrade momento sdk - consistency, less copypasta #150

Merged
merged 5 commits into from
May 21, 2022

Conversation

kvcache
Copy link
Contributor

@kvcache kvcache commented May 20, 2022

This change establishes a common pattern for interacting with the
service. Rather than copy/pasting matches, MomentoError -> CliError
mapping and json dump code around, this names those functionalities
as functions:

  • interact_with_momento: Completes a momento future and maps the
    result to a Result< HappyCaseType, CliError >
  • print_whatever_this_is_as_json: prints whatever you feed it as json

This change was precipitated by the change in the SDK to simplify
and coalesce client creation behind the SimpleCacheClientBuilder and
triggered by the SDK upgrade.

There are small changes to debug and error messages, but this change
should cause no functional changes.

tired of copypasta.

pub async fn delete_cache(cache_name: String, auth_token: String) -> Result<(), CliError> {
    debug!("deleting cache...");
    let mut momento = get_momento_client(auth_token).await?;
    match momento.delete_cache(&cache_name).await {
        Ok(_) => (),
        Err(e) => return Err(CliError { msg: e.to_string() }),
    };
    Ok(())
}

becomes

pub async fn delete_cache(cache_name: String, auth_token: String) -> Result<(), CliError> {
    let mut client = get_momento_client(auth_token).await?;
    interact_with_momento("deleting cache...", client.delete_cache(&cache_name)).await
}

This change establishes a common pattern for interacting with the
service. Rather than copy/pasting matches, MomentoError -> CliError
mapping and json dump code around, this names those functionalities
as functions:
* interact_with_momento: Completes a momento future and maps the
  result to a Result< HappyCaseType, CliError >
* print_whatever_this_is_as_json: prints whatever you feed it as json

There are small changes to debug and error messages, but this change
should cause no functional changes.
Copy link
Contributor

@tylerburdsall tylerburdsall left a comment

Choose a reason for hiding this comment

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

Minor comment, everything else looks good. Thank you for helping reduce our copy-pasta code, this is super clean

Comment on lines +26 to +34
pub fn print_whatever_this_is_as_json<T>(value: &T)
where
T: serde::Serialize,
{
println!(
"{}",
serde_json::to_string_pretty(value).expect("Could not print whatever this is as json")
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: I like having a function that uses generics to pretty print results, could we name it to something more streamlined like print_as_json? print_whatever_this_is almost seems a little tongue-in-cheek for a public-vended CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll remove this tiny island of joy from the sea of dispassion. I still need to look into windows too.

cprice404
cprice404 previously approved these changes May 20, 2022
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

nice cleanup, ty

@kvcache kvcache marked this pull request as draft May 20, 2022 23:05
@kvcache
Copy link
Contributor Author

kvcache commented May 20, 2022

converted to draft because I need to mess with the github workflow to make windows work. I don't know why it can't find cmake.

@kvcache kvcache marked this pull request as ready for review May 20, 2022 23:58
@kvcache
Copy link
Contributor Author

kvcache commented May 20, 2022

Integration tests are broken for unrelated reasons.

Copy link
Contributor

@bruuuuuuuce bruuuuuuuce left a comment

Choose a reason for hiding this comment

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

:shipit:

@kvcache kvcache merged commit 893c4d1 into main May 21, 2022
@kvcache kvcache deleted the stop_copypasta branch May 21, 2022 00:07
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