-
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
Open
SamuelMarks
wants to merge
16
commits into
Wulf:main
Choose a base branch
from
SamuelMarks:default-impl
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
5943f44
[src/code.rs] Implement generator for `impl Default` ; [src/{global.r…
SamuelMarks 25161ca
[src/code.rs] `None` as default when column is optional ; add `Partia…
SamuelMarks 797b323
[src/code.rs] Generate `impl Default for` CRUD structs. BUG: column n…
SamuelMarks 867c512
[src/code.rs] Optimise string allocations ; avoid `impl Default` on u…
SamuelMarks 9874dd5
[src/code.rs] Ignore generated_columns in `impl Default` blocks
SamuelMarks 0038da7
Merge branch 'main' into default-impl
SamuelMarks 5e19ba6
[src/code.rs] Use multiline raw string syntax
SamuelMarks ba8b38d
[src/code.rs] Apply changed from PR comments
SamuelMarks ed6b6c1
Merge branch 'main' into default-impl
SamuelMarks 176fb65
[src/code.rs] In `generate_for_table` use `create_struct.fields()` e…
SamuelMarks 4d31663
[src/code.rs] Use `.filter(not_generated)` on `create_struct` (`.fiel…
SamuelMarks 763c527
[src/code.rs] Add `impl Default for Update`
SamuelMarks b1be2b8
[src/code.rs] Comment out `impl Default for Update`
SamuelMarks 668266d
refactor(code): move "default_impl" code to "Struct::render"
hasezoey 532fa29
[src/code.rs] `build_default_impl_fn` now makes `None` the default fo…
SamuelMarks e617290
[src/code.rs] Add test for `build_default_impl_fn`
SamuelMarks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| 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}; | ||
|
|
@@ -86,7 +85,7 @@ pub struct StructField { | |
|
|
||
| impl StructField { | ||
| /// Assemble the current options into a rust type, like `base_type: String, is_optional: true` to `Option<String>` | ||
| pub fn to_rust_type(&self) -> Cow<str> { | ||
| pub fn to_rust_type(&self) -> std::borrow::Cow<'_, str> { | ||
| let mut rust_type = self.base_type.clone(); | ||
|
|
||
| // order matters! | ||
|
|
@@ -209,6 +208,7 @@ impl<'a> Struct<'a> { | |
| derives::SELECTABLE, | ||
| #[cfg(feature = "derive-queryablebyname")] | ||
| derives::QUERYABLEBYNAME, | ||
| derives::PARTIALEQ, | ||
| ]); | ||
|
|
||
| if !self.table.foreign_keys.is_empty() { | ||
|
|
@@ -761,10 +761,65 @@ fn build_imports(table: &ParsedTableMacro, config: &GenerationConfig) -> String | |
| imports_vec.join("\n") | ||
| } | ||
|
|
||
| /// Get default for type | ||
| 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()" | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Generate default (insides of the `impl Default for StructName { fn default() -> Self {} }`) | ||
| fn build_default_impl_fn<'a>( | ||
| struct_name: &str, | ||
| columns: impl Iterator<Item = &'a ParsedColumnMacro>, | ||
| ) -> String { | ||
| let column_name_type_nullable = | ||
| columns.map(|col| (col.name.to_string(), col.ty.as_str(), col.is_nullable)); | ||
| 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"); | ||
| format!( | ||
| r#"impl Default for {struct_name} {{ | ||
| fn default() -> Self {{ | ||
| Self {{ | ||
| {fields_to_defaults} | ||
| }} | ||
| }} | ||
| }}"# | ||
| ) | ||
hasezoey marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /// 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
Show resolved
Hide resolved
|
||
| let table_options = config.table(&table.name.to_string()); | ||
| let generated_columns = table_options.get_autogenerated_columns(); | ||
|
|
||
| let mut ret_buffer = format!("{FILE_SIGNATURE}\n\n"); | ||
|
|
||
|
|
@@ -777,9 +832,24 @@ pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) - | |
|
|
||
| let create_struct = Struct::new(StructType::Create, table, config); | ||
|
|
||
| let not_generated = |col: &&ParsedColumnMacro| -> bool { | ||
| !generated_columns.contains(&col.column_name.as_str()) | ||
| }; | ||
|
|
||
| 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( | ||
| &StructType::format(&StructType::Create, &struct_name), | ||
| create_struct.table.columns.iter().filter(not_generated), | ||
| ) | ||
| .as_str(), | ||
| ); | ||
| } | ||
|
||
| ret_buffer.push('\n'); | ||
SamuelMarks marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| let update_struct = Struct::new(StructType::Update, table, config); | ||
|
|
@@ -789,11 +859,20 @@ pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) - | |
| ret_buffer.push_str(update_struct.code()); | ||
| } | ||
|
|
||
| // 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.iter().filter(not_generated)) | ||
| .as_str(), | ||
| ); | ||
| ret_buffer.push('\n'); | ||
SamuelMarks marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| ret_buffer | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt this basically just return
Default::default()instead of manually mapping the defaults?There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
Default::default. It makes the code more readable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
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.
Choose a reason for hiding this comment
The 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
That could easily be rewritten:
…but is it more readable that way? - My code makes the generated code roughly as readable as what we write by hand.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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).
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.
Choose a reason for hiding this comment
The 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
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
::defaultinstead of the raw type. (at least i would expect it there)There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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:
String::default(),Vec::default(), etc.? - And for the scalars / literals, actual literals:That way developers will still see
defaultfunction calls everywhere but the oddfalseliteral; with a fallback toDefault::default()if it's not a builtin case I've manually pattern matched.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
Defaultderive macro usesfield: Default::default().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
constvalues.Playground example