-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
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.
Overall looks good! Most of my comments are about error messages and just a few questions I have 😄
src/commands/secret/mod.rs
Outdated
"Your wrangler.toml is likely missing the field \"account_id\", which is required to write to Workers KV." | ||
} | ||
10001 => "The content you passed in is not an accepted string. Try entering just a string", | ||
10053 => "There is already another binding with a different type by this name. Check your wrangler.toml or dashboard for conflicting bindings", |
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.
Did we decide we're listing secrets in wrangler.toml
? Don't want to re-litigate that discussion just want to make sure if we're telling people to check their toml that there's something they can do there.
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.
Also I'd say "wrangler.toml or your Cloudflare dashboard"
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.
so this happens in the case that there is a binding by the same name but of another kind, so perhaps a kv namespace or plain text binding, for example. this isn't in the case of a secret by the same name, because this is a set so it'll just overwrite.
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.
ah. how about "There is already a KV namespace or text binding with this name associated with your script"
or something like that/ "binding with a different type by this name" is hard to parse
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.
there is a world in which it is actually a text blob binding you are using for sites, or you went and called it wasm
or something else that's taken by our build process. if we wanted to be really robust, we could query the list of bindings to show what's up, check their toml, look to see if they're stomping on their own asset manifest or wasm binding... this feels like a good add-on issue though.
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.
👍
println!("{} [y/n]", prompt_string); | ||
let mut response: String = read!("{}\n"); | ||
response = response.split_whitespace().collect(); // remove whitespace | ||
response.make_ascii_lowercase(); // ensure response is all lowercase |
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.
is there a reason we use make_ascii_lowercase
instead of to_lowercase
?
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.
idk, @victoriabernard92 ?
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.
This was copy from gabbi's work on interactive delete
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.
interesting. it is oddly specific...perhaps we should leave it?
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.
i'm going to revert it to using this, and cc @gabbifish for when she gets back to see if we can change it. if not, i'd like to add a comment so we don't re-litigate this in the future.
b1168f4
to
c5e8970
Compare
869d978
to
62688b3
Compare
closes #907, #912, #909 . just a repackaging of #994