-
Notifications
You must be signed in to change notification settings - Fork 15
Implement generator for impl Default
#142
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
5943f44
25161ca
797b323
867c512
9874dd5
0038da7
5e19ba6
ba8b38d
ed6b6c1
176fb65
4d31663
763c527
b1be2b8
668266d
532fa29
e617290
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| use crate::parser::{ParsedColumnMacro, ParsedTableMacro, FILE_SIGNATURE}; | ||
| use crate::{get_table_module_name, GenerationConfig, TableOptions}; | ||
| use heck::ToPascalCase; | ||
| use indoc::formatdoc; | ||
| use std::borrow::Cow; | ||
|
|
||
| use crate::parser::{ParsedColumnMacro, ParsedTableMacro, FILE_SIGNATURE}; | ||
| use crate::{get_table_module_name, GenerationConfig, TableOptions}; | ||
| use std::iter::Map; | ||
| use std::slice::Iter; | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| enum StructType { | ||
|
|
@@ -209,6 +210,7 @@ impl<'a> Struct<'a> { | |
| derives::SELECTABLE, | ||
| #[cfg(feature = "derive-queryablebyname")] | ||
| derives::QUERYABLEBYNAME, | ||
| derives::PARTIALEQ, | ||
| ]); | ||
|
|
||
| if !self.table.foreign_keys.is_empty() { | ||
|
|
@@ -761,9 +763,58 @@ fn build_imports(table: &ParsedTableMacro, config: &GenerationConfig) -> String | |
| imports_vec.join("\n") | ||
| } | ||
|
|
||
| /// Get default for type | ||
| fn default_for_type(typ: String) -> &'static str { | ||
| match typ.as_str() { | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| "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" => "\"\"", | ||
| _ => "Default::default()", | ||
| } | ||
| } | ||
|
Comment on lines
+776
to
+794
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldnt this basically just return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could, but throughout your codebase—for example—you are using more specific calls over
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Note that i am talking about the generated code not our code style.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes that could be written as such and i dont think we enforce either style there and either is fine (unless specifically needed). Also now that i think about it, why is it implemented manually if a derive could just be added?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: That way developers will still see
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes for similar cases as yours where dsync can generate the boilerplate, but can be customized afterward - if necessary.
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 In any case, it is fine as it is now and can be changed in a follow-up PR.
I dont see how the error messages are relevant for this case. Those Error messages are explicitly for |
||
|
|
||
| /// Generate default (insides of the `impl Default for StructName { fn default() -> Self {} }`) | ||
| fn build_default_impl_fn(struct_name: &str, columns: &Vec<ParsedColumnMacro>) -> String { | ||
| let mut buffer = String::with_capacity(struct_name.len() + columns.len() * 4); | ||
| buffer.push_str(&format!( | ||
| "impl Default for {struct_name} {{\n fn default() -> Self {{\n", | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| struct_name = struct_name | ||
| )); | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| let column_name_type_nullable: Map< | ||
| Iter<ParsedColumnMacro>, | ||
| fn(&ParsedColumnMacro) -> (String, String, bool), | ||
| > = columns | ||
| .iter() | ||
| .map(|col| (col.name.to_string(), col.ty.to_string(), col.is_nullable)); | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
|
|
||
| buffer.push_str(" Self {\n"); | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| let fields_to_defaults = column_name_type_nullable | ||
| .map(|(name, typ, nullable)| { | ||
| format!( | ||
| " {name}: {typ_default}", | ||
| name = name, | ||
| typ_default = if nullable { | ||
| "None" | ||
| } else { | ||
| default_for_type(typ) | ||
| } | ||
| ) | ||
| }) | ||
| .collect::<Vec<String>>() | ||
| .join(",\n"); | ||
| buffer.push_str(fields_to_defaults.as_str()); | ||
| buffer.push_str("\n }\n }\n}"); | ||
| buffer | ||
| } | ||
|
|
||
| /// Generate a full file for a given diesel table | ||
| pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) -> String { | ||
| // early to ensure the table options are set for the current table | ||
| let struct_name = table.struct_name.to_string(); | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| let table_options = config.table(&table.name.to_string()); | ||
|
|
||
| let mut ret_buffer = format!("{FILE_SIGNATURE}\n\n"); | ||
|
|
@@ -780,20 +831,48 @@ pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) - | |
| if create_struct.has_code() { | ||
| ret_buffer.push('\n'); | ||
| ret_buffer.push_str(create_struct.code()); | ||
| if config.options.default_impl { | ||
| ret_buffer.push('\n'); | ||
| ret_buffer.push_str( | ||
| build_default_impl_fn( | ||
| &format!("Create{struct_name}"), | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| &create_struct.table.columns, | ||
| ) | ||
| .as_str(), | ||
| ); | ||
| } | ||
| ret_buffer.push('\n'); | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| let update_struct = Struct::new(StructType::Update, table, config); | ||
|
|
||
| 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}"), | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| &update_struct.table.columns, | ||
| ) | ||
| .as_str(), | ||
| ); | ||
| } | ||
| ret_buffer.push('\n'); | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| // third and lastly, push functions - if enabled | ||
| // third, push functions - if enabled | ||
| if table_options.get_fns() { | ||
| ret_buffer.push('\n'); | ||
| ret_buffer.push_str(build_table_fns(table, config, create_struct, update_struct).as_str()); | ||
| } | ||
|
|
||
| if config.options.default_impl { | ||
| ret_buffer.push('\n'); | ||
| ret_buffer.push_str(build_default_impl_fn(&struct_name, &table.columns).as_str()); | ||
| ret_buffer.push('\n'); | ||
|
SamuelMarks marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| ret_buffer | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.