Implement generator for impl Default
#142
Conversation
…s,bin/main.rs}] Expose new generator all the way up to binary
…ames don't match struct
|
@Wulf Something weird is happening on my latest commit: if update_struct.has_code() {
ret_buffer.push('\n');
ret_buffer.push_str(update_struct.code());
if config.options.default_impl {
ret_buffer.push('\n');
ret_buffer.push_str(
build_default_impl_fn(
&format!("Update{struct_name}"),
&update_struct.table.columns,
)
.as_str(),
);
}
ret_buffer.push('\n');
}The |
hasezoey
left a comment
hasezoey
left a comment
There was a problem hiding this comment.
Please add tests (see simple_table_sqlite as a example), aside from that some minor style improvements.
|
btw, we dont have a CONTRIBUTING currently, but the commit style is angular/conventional-ish, see the commit history for examples. |
…pdate ; use `StructType::format`
|
Good suggestions. Yeah I still need to add tests, but it should be ready now (aside from that). |
There was a problem hiding this comment.
Looks better, but still the tests are missing (at least one for being enabled with most common types).
Also consider using r# and multiple lines instead of \n for the format! (like other code in the file), example:
Lines 497 to 515 in 2797966
PS: also existing test output needs to be updated if it is meant to change. (though to my understanding it shouldnt change as long as default_impl is false, which it is by default)
|
@hasezoey Hmm maybe I'm misusing something, let generated_columns = table_options.get_autogenerated_columns();
println!("generated_columns = {generated_columns:#?}"); |
hasezoey
left a comment
hasezoey
left a comment
There was a problem hiding this comment.
Code looks mostly OK, but most importantly, tests need to be updated and new tests added for this new option.
Hmm maybe I'm misusing something, generated_columns = [] always in generate_for_table of src/code.rs. The rest of the code works well though.
I dont know how you are using it, but that will always be empty unless you specify the generated columns in TableOptions::autogenerated_columns / --autogenerated-columns on the CLI (MainOptions).
| fn default_for_type(typ: &str) -> &'static str { | ||
| match typ { | ||
| "i8" | "u8" | "i16" | "u16" | "i32" | "u32" | "i64" | "u64" | "i128" | "u128" | "isize" | ||
| | "usize" => "0", | ||
| "f32" | "f64" => "0.0", | ||
| // https://doc.rust-lang.org/std/primitive.bool.html#method.default | ||
| "bool" => "false", | ||
| "String" => "String::new()", | ||
| "&str" | "&'static str" => "\"\"", | ||
| "Cow<str>" => "Cow::Owned(String::new())", | ||
| _ => { | ||
| if typ.starts_with("Option<") { | ||
| "None" | ||
| } else { | ||
| "Default::default()" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Shouldnt this basically just return Default::default() instead of manually mapping the defaults?
There was a problem hiding this comment.
It could, but throughout your codebase—for example—you are using more specific calls over Default::default. It makes the code more readable?
There was a problem hiding this comment.
I dont recall that we do this, so i had a quick skim again and couldnt find anything related that we are doing this for Defaults. Do you have a example of where we do this?
Note that i am talking about the generated code not our code style.
There was a problem hiding this comment.
Here for example https://github.com/Wulf/dsync/blob/2797966/src/global.rs#L356-L368
impl Default for GenerationConfigOpts<'_> {
fn default() -> Self {
Self {
table_options: HashMap::default(),
default_table_options: Default::default(),
schema_path: String::from(DEFAULT_SCHEMA_PATH),
model_path: String::from(DEFAULT_MODEL_PATH),
once_common_structs: false,
once_connection_type: false,
readonly_prefixes: Vec::default(),
readonly_suffixes: Vec::default(),
}
}
}That could easily be rewritten:
impl Default for GenerationConfigOpts<'_> {
fn default() -> Self {
Self {
table_options: Default::default(),
default_table_options: Default::default(),
schema_path: String::from(DEFAULT_SCHEMA_PATH),
model_path: String::from(DEFAULT_MODEL_PATH),
once_common_structs: Default::default(),
once_connection_type: Default::default(),
readonly_prefixes: Default::default(),
readonly_suffixes: Default::default(),
}
}
}…but is it more readable that way? - My code makes the generated code roughly as readable as what we write by hand.
There was a problem hiding this comment.
That could easily be rewritten:
Yes that could be written as such and i dont think we enforce either style there and either is fine (unless specifically needed).
But like i said, i am talking about generated code not about our code style of dsync itself. This example is not generated code.
Also now that i think about it, why is it implemented manually if a derive could just be added?
There was a problem hiding this comment.
Yes a derive could be written from this and it would mostly work. Not sure if it'd handle the edge cases, like timezone specific starts and whatnot.
If you just want to take the derive approach, then merge my other PR and close this one.
Personally, I like how explicit this one is, and it futureproofs the tz and other more interesting use-cases that can be inferred from db schema but not necessarily other parts of the codebase.
There was a problem hiding this comment.
If you just want to take the derive approach, then merge my other PR and close this one.
Yes that could be done, but i also see the point of dsync generating the schema once and users manually modifying it afterward, so i this PR still has its use. Just was curious what your use-case for this PR not doing a simple derive was.
Still though, i think from a maintenance point of view Default::default() for all the fields would be the best idea, unless there is a type that does not have that or, as you already said, to customize some types (like timezones).
This means that if there should ever be a change for the default impl, we dont have to also change it, and i think a user would expect to actually see ::default instead of the raw type. (at least i would expect it there)
There was a problem hiding this comment.
Ohh interesting, so you expect people to modify the code after it's generated? - I mean, I do, but that's just because you lack the features I need (and I'm sending you PRs to fix).
How about a compromise: String::default(), Vec::default(), etc.? - And for the scalars / literals, actual literals:
--> src/main.rs:18:29
|
18 | const S: &'static str = Default::default();
| ^^^^^^^^^^^^^^^^^^
|
= note: calls in constants are limited to constant functions, tuple structs and tuple variants
error[E0015]: cannot call non-const associated function `<u8 as Default>::default` in constants
--> src/main.rs:19:20
|
19 | const uu: u8 = Default::default();
| ^^^^^^^^^^^^^^^^^^
That way developers will still see default function calls everywhere but the odd false literal; with a fallback to Default::default() if it's not a builtin case I've manually pattern matched.
There was a problem hiding this comment.
Ohh interesting, so you expect people to modify the code after it's generated?
Yes for similar cases as yours where dsync can generate the boilerplate, but can be customized afterward - if necessary.
How about a compromise: String::default(), Vec::default(), etc.?
Sure, but personally, i would expect the same behavior as if using rust-analyzer's "expand derive macro", which practically takes the generated code from the derive and dumps it after the struct, and the Default derive macro uses field: Default::default().
In any case, it is fine as it is now and can be changed in a follow-up PR.
And for the scalars / literals, actual literals:
I dont see how the error messages are relevant for this case. Those Error messages are explicitly for const values.
Playground example
src/code.rs
Outdated
| if config.options.default_impl { | ||
| ret_buffer.push('\n'); | ||
| ret_buffer.push_str( | ||
| build_default_impl_fn( | ||
| &StructType::format(&StructType::Create, &struct_name), | ||
| create_struct.table.columns.iter().filter(not_generated), | ||
| ) | ||
| .as_str(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This should arguably be in Struct::render.
There was a problem hiding this comment.
Curious to see what that would look like
Yeah I am using those though. Maybe I'll try constructing a test so you can see. |
# Conflicts: # src/code.rs
…se `table.columns` as before
…ds` was being weird)
|
@hasezoey Darn… trying to get Attempt 0:build_default_impl_fn(
&StructType::format(&StructType::Update, &struct_name),
update_struct
.table
.columns
.iter()Attempt 1:build_default_impl_fn(
&StructType::format(&StructType::Update, &struct_name),
update_struct
.fields()Both attempts give the wrong fields, basically: /// Update Struct for a row in table `users` for [`Users`]
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::AsChangeset, PartialEq)]
#[diesel(table_name=users)]
pub struct UpdateUsers {
/// Field representing column `password_hash`
pub password_hash: Option<String>,
/// Field representing column `role`
pub role: Option<String>,
/// Field representing column `created_at`
pub created_at: Option<chrono::NaiveDateTime>,
}
impl Default for UpdateUsers {
fn default() -> Self {
Self {
username: String::new(),
password_hash: String::new(),
role: String::new()
}
}
}FWIW: The other default is correct /// Create Struct for a row in table `users` for [`Users`]
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::Insertable)]
#[diesel(table_name=users)]
pub struct CreateUsers {
/// Field representing column `username`
pub username: String,
/// Field representing column `password_hash`
pub password_hash: String,
/// Field representing column `role`
pub role: String,
}
impl Default for CreateUsers {
fn default() -> Self {
Self {
username: String::new(),
password_hash: String::new(),
role: String::new()
}
}
}So clearly the columns are not being properly set for this struct. How do I get the correctly filtered columns for the given table? |
I dont know what you are testing against as you still did not provide a test case with this new option, and with my slight refactor and previous suggestion, i dont see any problem, see the attached patch. Patch that moves the generation into
|
|
@Wulf Thanks. Made an edit to handle the Update properly, what do you think now, can we get this merged in? - Then the other PR? Then I've got some big ideas to try, but likely beyond dsync scope… |
And add test for it
…r all Update fields
hasezoey
left a comment
hasezoey
left a comment
There was a problem hiding this comment.
Looks mostly good to me now, but the tests still need to be updated.
Might i recommend, for reviewability, to put the PartialEq change in a different PR?
Also consider adding a CHANGELOG entry (it is not automated yet as semantic-release only works with 1.0.0+ versions)
| impl Default for UpdateTodos { | ||
| fn default() -> Self { | ||
| Self { | ||
| text: String::new(), | ||
| completed: false, | ||
| type_: String::new(), | ||
| smallint: 0, | ||
| bigint: 0, | ||
| created_at: Default::default(), | ||
| updated_at: Default::default(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The tests need to be updated.
|
@hasezoey Can you help this get over the line? Then the other PR? - Then I want to send a new PR to implement |
I dont know what to help you with aside from repeating what i said earlier in this PR (also applies to your other PR):
In case you had not seen or done it before in this project, to update the tests you need to follow these steps (relative to the project root):
Also due to the amount of changes the |
2 participants
No description provided.