Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add options "create-str" and "update-str" #83

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

hasezoey
Copy link
Collaborator

This PR adds options create-str and update-str to set which string type to use, this can be useful over just String, because those structs may not need to own their values, but the normal (StructType::Read) will still need to own it values

re #56

@Wulf
Copy link
Owner

Wulf commented Sep 29, 2023

Looks good to me @hasezoey, could I request that we simplify the tests by only including a string field for the Todo model and nothing else? If you think it provides value, I'm okay with it 🙌 !

@hasezoey
Copy link
Collaborator Author

could I request that we simplify the tests by only including a string field for the Todo model and nothing else?

sure, i basically just always copy simple_table as a go-to test

@hasezoey hasezoey force-pushed the strType branch 2 times, most recently from 3f1c51d to 9fa54f7 Compare September 30, 2023 18:13
@hasezoey
Copy link
Collaborator Author

Update:

  • changed test schema to only include necessary fields
  • added some extra fields (nullable) for testing
  • noticed that nullable was not correctly handled, and fixed it (as a side-effect Update structs are now Option<TYPE> instead of Option<Option<TYPE>>)

@hasezoey hasezoey requested a review from Wulf September 30, 2023 18:17
@Wulf
Copy link
Owner

Wulf commented Oct 1, 2023

noticed that nullable was not correctly handled, and fixed it (as a side-effect Update structs are now Option instead of Option<Option>)

Can you give an example here? We don't want to remove Option<Option<TYPE>> because that makes the API less flexible.

Currently, you can update the fields you're interested in and set None for the rest.

Here's an example:

Imagine we had this table:

diesel::table! {
    todos (id) {
        text -> Text,
        text_nullable -> Nullable<Text>,
    }
}

Here's what dsync will generate:

pub struct UpdateTodo {
    pub text: Option<String>,
    pub text_nullable: Option<Option<String>>,
}

And here are some example queries:

  1. This will not update any of the fields:

        Todo::update(.., .., &UpdateTodo {
            text: None,
            text_nullable: None,
        });
  2. This will only update the 'text' field:

        Todo::update(.., .., &UpdateTodo {
            text: Some("updated!".to_string()),
            text_nullable: None,
        });
  3. This will set text_nullable to NULL

        Todo::update(.., .., &UpdateTodo {
            text: None,
            text_nullable: Some(None),
        });
  4. This will set text_nullable:

        Todo::update(.., .., &UpdateTodo {
            text: None,
            text_nullable: Some(Some("updated!".to_string())),
        });

@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 1, 2023

Can you give an example here? We don't want to remove Option<Option> because that makes the API less flexible.

thanks for pointing this out, i will fix it.

the problem i fixed with that was that base_type(which to my knowledge is meant to actually be the base type) was set to Option<BASE_TYPE> within fields(), and then set again in render(), resulting that checks on base_type would fail (ie base_type == "String") because it was already Option<String>, but there was also the extra property of is_optional which was set and so resulted in Option<Option<BASE_TYPE>>.

please clarify on what function fields() and render()'s purposes are, because currently they handle somewhat of many things, like fields() will set is_optional if the struct type is Update, but so losing the original is_optional, previously it also wrapped base_type in Option and render wrapped it again

example of fixing the current state:

(context this line)

-let field_type = if f.is_optional {
+let mut field_type = if f.is_optional {
    format!("Option<{}>", base_type)
} else {
    base_type.into()
};

+if self.ty == StructType::Update {
+    field_type = format!("Option<{}>", field_type);
+}

but then we would also need to remove the following lines:

dsync/src/code.rs

Lines 199 to 206 in 9fa54f7

match self.ty {
StructType::Read => {}
StructType::Update => {
// all non-key fields should be optional in Form structs (to allow partial updates)
is_optional = !is_pk || is_autogenerated;
}
StructType::Create => {}
}

because otherwise (as mentioned earlier) fields() will always set is_optional if struct-type is Update, and so changing all properties (regardless of nullable) to be Option<Option<BASE_TYPE>>

or do know a better solution?

i will make a separate PR (and let this PR depend on it) to try to refactor how those functions work

hasezoey added a commit to hasezoey/dsync that referenced this pull request Oct 1, 2023
@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 1, 2023

i will make a separate PR (and let this PR depend on it) to try to refactor how those functions work

the PR is now created as #85

Blocked until #85 is merged / a other solution is found

hasezoey added a commit to hasezoey/dsync that referenced this pull request Oct 1, 2023
@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 4, 2023

PR #85 is merged, and this PR has been rebased to use the merged code, dropping the problematic commit (also adjusting for changes from that PR)

@Wulf is this PR good now?

Copy link
Owner

@Wulf Wulf left a comment

Choose a reason for hiding this comment

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

@hasezoey looks good to me :-)
🙌

@hasezoey hasezoey merged commit 33ea467 into Wulf:main Oct 8, 2023
@hasezoey hasezoey deleted the strType branch October 8, 2023 12:19
@hasezoey hasezoey mentioned this pull request Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants