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

Don't break wrangler preview when kv namespaces present #353

Merged
merged 3 commits into from
Jul 26, 2019

Conversation

ashleymichal
Copy link
Contributor

the preview service as we use it today does not ignore kv namespace bindings, but throws an error when sent a script with those bindings. we don't want to take away a user's ability to use preview, for one, and we also want to give a helpful warning when they do.

@ashleymichal ashleymichal added changelog - feature regression Something is broken, but works in previous releases labels Jul 23, 2019
@ashleymichal ashleymichal added this to the KV Namespace Config milestone Jul 23, 2019
pub fn build_script_upload_form(
project: &Project,
include_kv: bool,
) -> Result<Form, failure::Error> {
Copy link
Contributor

@steveklabnik steveklabnik Jul 24, 2019

Choose a reason for hiding this comment

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

so, boolean parameters are generally considered to be a bad style. I actually really like what you did above, with

    let include_kv = false;

rather than just passing false and true. But we can do even better!

// note this isn't pub!
fn build_script_upload_form_with_namespaces(
    project: &Project,
    kv_namespaces: Vec<NamespaceTypeOrWhatever>,
) -> Result<Form, failure::Error> {
   // previous body of build_script_upload_form goes here
   //
   // except you don't need to set up the kv_namespaces variable
}


pub fn build_script_upload_form(
    project: &Project,
) -> Result<Form, failure::Error> {
    build_script_upload_form_with_namespaces(project, Vec::new())
}

pub fn build_script_upload_form_with_kv(
    project: &Project,
) -> Result<Form, failure::Error> {
     build_script_upload_form_with_namespaces(project, project.kv_namespaces())
}

I do not know how @ashleygwilliams feels about this kind of refactoring, but I generally prefer it. That being said, boolean parameters are not the worst, so if she prefers this code as-is, I don't think it's the end of the world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the boolean parameter did indeed feel Gross. I like this better, ty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented this, except called out the "weird" case which is building without kv.

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.

Looking good! Great call on reversing the names.

@ashleymichal ashleymichal merged commit ab67eec into feat-kv-config Jul 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the alewis/preview-kv branch July 26, 2019 19:46
@ashleymichal ashleymichal restored the alewis/preview-kv branch August 12, 2019 14:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression Something is broken, but works in previous releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants