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

feat: Support KV Namespace Configuration #334

Merged
merged 11 commits into from
Jul 29, 2019
Merged

Conversation

ashleymichal
Copy link
Contributor

@ashleymichal ashleymichal commented Jul 19, 2019

Builds on #329 (merge that first)

Closes #273 #92

@ashleymichal
Copy link
Contributor Author

rebased onto master. no merge conflicts

@ashleymichal ashleymichal force-pushed the feat-kv-config branch 2 times, most recently from ddb663c to 34b3e60 Compare July 23, 2019 18:22
Copy link

@cwndrws cwndrws left a comment

Choose a reason for hiding this comment

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

LGTM. Added a few nit comments, but you can ignore if you don't agree.

src/commands/publish/upload_form/mod.rs Show resolved Hide resolved
src/commands/publish/upload_form/mod.rs Outdated Show resolved Hide resolved
src/commands/publish/upload_form/mod.rs Outdated Show resolved Hide resolved
src/settings/binding.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

Just some small stuff; a few suggestions. Looks great overall!

src/commands/publish/upload_form/project_assets.rs Outdated Show resolved Hide resolved
src/commands/publish/upload_form/wasm_module.rs Outdated Show resolved Hide resolved
src/settings/project/kv_namespace.rs Outdated Show resolved Hide resolved
match &self.kv_namespaces {
Some(kv) => kv.clone(),
None => Vec::new(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be written

self.kv_namespaces.cloned().unwrap_or_else(Vec::new)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i hope you meant .clone() bc .cloned() gave me many complaints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

image

cloned goes from Option<&T> to Option<T>. But you already have an Option<T>. clone is right here, and I'm wrong.

Thanks, compiler!

src/settings/project/mod.rs Outdated Show resolved Hide resolved
@kristianfreeman
Copy link
Contributor

@ashleymichal can you add some details about what this PR is, so that I can document it in the CHANGELOG when it's merged?

@@ -143,6 +143,17 @@ There are two types of configuration that `wrangler` uses: global user and per p
This key is optional if you are using a workers.dev subdomain and is only required for `publish --release`.
- `webpack_config`: This is the path to the webpack configuration file for your worker. This is optional and
defaults to `webpack.config.js`
- `[[kv-namespaces]]`: These specify any [Workers KV](https://workers.cloudflare.com/docs/reference/storage/) namespaces you want to access from
Copy link
Contributor

Choose a reason for hiding this comment

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

we should explicitly call out that users are expected to make their namespace in the UI

Copy link
Contributor Author

@ashleymichal ashleymichal Jul 29, 2019

Choose a reason for hiding this comment

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

added commit for this, also points to docs showing API call.

* use serde renames instead of silencing clippy warning in Binding
* derive PartialEq for KvNamespace rather than impl'ing manually
* pass &Path instead of PathBuf to get_project_config
* unwrap_or_else kv_namespaces instead of matching
@ashleymichal ashleymichal merged commit d54dc88 into master Jul 29, 2019
@delete-merged-branch delete-merged-branch bot deleted the feat-kv-config branch July 29, 2019 20:14
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.

Refactor Bindings to support all binding types
5 participants