-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
6404086
to
0ed02e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'm applying some of Steve's feedback from his first pass at our feat-kv-commands branch, as well as other areas where we can simplify our Rust logic.
also need to update documentation. |
Co-Authored-By: Gabbi Fisher <[email protected]>
src/commands/kv/bucket.rs
Outdated
Ok(key_value_pairs | ||
.iter() | ||
.map(|kv| kv.key.clone()) | ||
.collect::<Vec<_>>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still think we can get rid of this cloning logic? I think it would be a good idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, apparently if it isn't in a PR comment I don't remember it.
pub fn upload(namespace_id: &str, filename: &Path) -> Result<(), failure::Error> { | ||
let pairs: Result<Vec<KeyValuePair>, failure::Error> = match &metadata(filename) { | ||
Ok(file_type) if file_type.is_dir() => directory_keys_values(filename), | ||
Ok(_file_type) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok(_file_type) => { | |
Ok(_) => { |
📦
Ok(_) => { | ||
// any other file types (namely, symlinks) | ||
failure::bail!( | ||
"{} should be a file or directory, but is a symlink", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"{} should be a file or directory, but is a symlink", | |
"wrangler kv:bucket delete takes a directory", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nits but otherwise LGTM!!
Closes #512 , Closes #513