Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Alewis/commands refactor kv #1332

Merged
merged 14 commits into from
Jun 2, 2020
Merged

Conversation

ashleymichal
Copy link
Contributor

@ashleymichal ashleymichal commented May 27, 2020

this looks like a nightmare, and that's a little bit because it is. but this gets most of the separation of concerns between kv implementation and the commands, as well as giving site specific logic its own home.

@ashleymichal ashleymichal marked this pull request as ready for review June 2, 2020 17:41
@ashleymichal ashleymichal requested a review from a team as a code owner June 2, 2020 17:41
src/kv/bulk.rs Outdated
@@ -0,0 +1,67 @@
extern crate base64;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the point of extern crate? that was deprecated in rust 2018

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk! i thought i had dropped this but it maybe snuck back in? can update

src/kv/bulk.rs Outdated
bulk_key_value_pairs: pairs.to_owned(),
}) {
Ok(_) => Ok(()),
Err(e) => failure::bail!("{}", kv::format_error(e)),
Copy link
Contributor

Choose a reason for hiding this comment

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

in another section, we imported this and just called format_error - lets make em consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh i hate that this is hereeeeeeeee next stop thiserror

src/kv/bulk.rs Outdated

match response {
Ok(_) => Ok(()),
Err(e) => failure::bail!("{}", kv::format_error(e)),
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@@ -9,7 +9,7 @@ use std::path::PathBuf;

use serde::{Deserialize, Serialize};

#[derive(Clone, Debug, Deserialize, Serialize)]
#[derive(Clone, Debug, Default, Deserialize, Serialize)]
Copy link
Contributor

Choose a reason for hiding this comment

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

what do we use Default for? what is a Default target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this snuck in while i was doing some work with prototyping on the deployments project. all a default here would be is each field would be set to its type's default -- empty String, None for Option, etc. deriving this here is just a convenience, i can leave it or not.

@ashleymichal ashleymichal merged commit 1658b76 into master Jun 2, 2020
@ashleymichal ashleymichal deleted the alewis/commands-refactor-kv branch June 2, 2020 19:01
@ispivey ispivey added this to the 1.10.0 milestone Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants