-
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 15 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
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 |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| [lib] | ||
| path = "lib.rs" | ||
|
|
||
| [package] | ||
| name = "default_impl" | ||
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [dependencies] | ||
| diesel = { version = "*", default-features = false, features = [ | ||
| "sqlite", | ||
| "r2d2", | ||
| "chrono", | ||
| "returning_clauses_for_sqlite_3_35", | ||
| ] } | ||
| r2d2.workspace = true | ||
| chrono.workspace = true | ||
| serde.workspace = true |
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 |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| pub mod models; | ||
| pub mod schema; | ||
|
|
||
| pub mod diesel { | ||
| pub use diesel::*; | ||
| } |
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| pub mod todos; |
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 |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| /* @generated and managed by dsync */ | ||
|
|
||
| #[allow(unused)] | ||
| use crate::diesel::*; | ||
| use crate::schema::*; | ||
|
|
||
| pub type ConnectionType = diesel::r2d2::PooledConnection<diesel::r2d2::ConnectionManager<diesel::sqlite::SqliteConnection>>; | ||
|
|
||
| /// Struct representing a row in table `todos` | ||
| #[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::Queryable, diesel::Selectable, diesel::QueryableByName, PartialEq, diesel::Identifiable)] | ||
| #[diesel(table_name=todos, primary_key(id))] | ||
| pub struct Todos { | ||
| /// Field representing column `id` | ||
| pub id: i32, | ||
| /// Field representing column `text` | ||
| pub text: String, | ||
| /// Field representing column `completed` | ||
| pub completed: bool, | ||
| /// Field representing column `type` | ||
| pub type_: String, | ||
| /// Field representing column `smallint` | ||
| pub smallint: i16, | ||
| /// Field representing column `bigint` | ||
| pub bigint: i64, | ||
| /// Field representing column `created_at` | ||
| pub created_at: chrono::NaiveDateTime, | ||
| /// Field representing column `updated_at` | ||
| pub updated_at: chrono::NaiveDateTime, | ||
| } | ||
|
|
||
| impl Default for Todos { | ||
| fn default() -> Self { | ||
| Self { | ||
| id: 0, | ||
| text: String::new(), | ||
| completed: false, | ||
| type_: String::new(), | ||
| smallint: 0, | ||
| bigint: 0, | ||
| created_at: Default::default(), | ||
| updated_at: Default::default(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Create Struct for a row in table `todos` for [`Todos`] | ||
| #[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::Insertable)] | ||
| #[diesel(table_name=todos)] | ||
| pub struct CreateTodos { | ||
| /// Field representing column `text` | ||
| pub text: String, | ||
| /// Field representing column `completed` | ||
| pub completed: bool, | ||
| /// Field representing column `type` | ||
| pub type_: String, | ||
| /// Field representing column `smallint` | ||
| pub smallint: i16, | ||
| /// Field representing column `bigint` | ||
| pub bigint: i64, | ||
| } | ||
|
|
||
| impl Default for CreateTodos { | ||
| fn default() -> Self { | ||
| Self { | ||
| text: String::new(), | ||
| completed: false, | ||
| type_: String::new(), | ||
| smallint: 0, | ||
| bigint: 0, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Update Struct for a row in table `todos` for [`Todos`] | ||
| #[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::AsChangeset, PartialEq)] | ||
| #[diesel(table_name=todos)] | ||
| pub struct UpdateTodos { | ||
| /// Field representing column `text` | ||
| pub text: Option<String>, | ||
| /// Field representing column `completed` | ||
| pub completed: Option<bool>, | ||
| /// Field representing column `type` | ||
| pub type_: Option<String>, | ||
| /// Field representing column `smallint` | ||
| pub smallint: Option<i16>, | ||
| /// Field representing column `bigint` | ||
| pub bigint: Option<i64>, | ||
| /// Field representing column `created_at` | ||
| pub created_at: Option<chrono::NaiveDateTime>, | ||
| /// Field representing column `updated_at` | ||
| pub updated_at: Option<chrono::NaiveDateTime>, | ||
| } | ||
|
|
||
| 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(), | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+94
to
+106
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. The tests need to be updated. |
||
|
|
||
| /// Result of a `.paginate` function | ||
| #[derive(Debug, serde::Serialize)] | ||
| pub struct PaginationResult<T> { | ||
| /// Resulting items that are from the current page | ||
| pub items: Vec<T>, | ||
| /// The count of total items there are | ||
| pub total_items: i64, | ||
| /// Current page, 0-based index | ||
| pub page: i64, | ||
| /// Size of a page | ||
| pub page_size: i64, | ||
| /// Number of total possible pages, given the `page_size` and `total_items` | ||
| pub num_pages: i64, | ||
| } | ||
|
|
||
| impl Todos { | ||
| /// Insert a new row into `todos` with a given [`CreateTodos`] | ||
| pub fn create(db: &mut ConnectionType, item: &CreateTodos) -> diesel::QueryResult<Self> { | ||
| use crate::schema::todos::dsl::*; | ||
|
|
||
| diesel::insert_into(todos).values(item).get_result::<Self>(db) | ||
| } | ||
|
|
||
| /// Get a row from `todos`, identified by the primary key | ||
| pub fn read(db: &mut ConnectionType, param_id: i32) -> diesel::QueryResult<Self> { | ||
| use crate::schema::todos::dsl::*; | ||
|
|
||
| todos.filter(id.eq(param_id)).first::<Self>(db) | ||
| } | ||
|
|
||
| /// Update a row in `todos`, identified by the primary key with [`UpdateTodos`] | ||
| pub fn update(db: &mut ConnectionType, param_id: i32, item: &UpdateTodos) -> diesel::QueryResult<Self> { | ||
| use crate::schema::todos::dsl::*; | ||
|
|
||
| diesel::update(todos.filter(id.eq(param_id))).set(item).get_result(db) | ||
| } | ||
|
|
||
| /// Delete a row in `todos`, identified by the primary key | ||
| pub fn delete(db: &mut ConnectionType, param_id: i32) -> diesel::QueryResult<usize> { | ||
| use crate::schema::todos::dsl::*; | ||
|
|
||
| diesel::delete(todos.filter(id.eq(param_id))).execute(db) | ||
| } | ||
| } | ||
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 |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| pub use generated::*; | ||
| pub mod generated; |
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 |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| diesel::table! { | ||
| todos (id) { | ||
| id -> Int4, | ||
| // unsigned -> Unsigned<Integer>, | ||
| // unsigned_nullable -> Nullable<Unsigned<Integer>>, | ||
| text -> Text, | ||
| completed -> Bool, | ||
| #[sql_name = "type"] | ||
| #[max_length = 255] | ||
| type_ -> Varchar, | ||
| smallint -> Int2, | ||
| bigint -> Int8, | ||
| created_at -> Timestamp, | ||
| updated_at -> Timestamp, | ||
| } | ||
| } |
Oops, something went wrong.
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