Remove impl_Insertable macro #692
Conversation
weiznich
force-pushed
the
remove_insertable_macro
branch
3 times, most recently
from
446e48e
to
e94f313
Compare
There was a problem hiding this comment.
Before digging into the functional aspects of this, I left some stylistic requests. Formatting complex types is not easy, but I hope you can see a pattern in my suggestions. (Maybe someone over at rust-lang/style-team#15 is interested in this as well?) I'd also appreciate trailing commas everywhere (I've commented only in a few places).
Feel free to not make the code style changes until after the functionality has been reviewed.
diesel/src/migrations/schema.rs
Outdated
| (ColumnInsertValue<self::__diesel_schema_migrations::version, | ||
| AsNullableExpr<&'insert &'a str, | ||
| self::__diesel_schema_migrations::version>>, ): InsertValues<DB>, | ||
| { |
There was a problem hiding this comment.
This is pretty much unparsable to me. Maybe format it like this?
type Table = self::__diesel_schema_migrations::table;
type Version = self::__diesel_schema_migrations::version;
impl<'ins, 'a, DB> Insertable<Table, DB> for &'ins NewMigration<'a> where
DB: Backend,
(ColumnInsertValue<
Version,
AsNullableExpr<&'ins &'a str, version>,
>,): InsertValues<DB>,
diesel_codegen/src/insertable.rs
Outdated
| ColumnInsertValue::Expression( | ||
| #column, | ||
| AsExpression::<<<#column as Expression>::SqlType | ||
| as IntoNullable>::Nullable>::as_expression(value) |
There was a problem hiding this comment.
Can you try not to break the line here? I'd split this up into
type NullableColumn = AsExpression::<
<<#column as Expression>::SqlType as IntoNullable>::Nullable,
>;and then use NullableColumn::as_expression(value)
diesel/src/migrations/schema.rs
Outdated
| (ColumnInsertValue::Expression(self::__diesel_schema_migrations::version, | ||
| AsExpression::<<<self::__diesel_schema_migrations::version | ||
| as Expression>::SqlType as IntoNullable>::Nullable> | ||
| ::as_expression(version)),) |
There was a problem hiding this comment.
Can you try not to break this at as and ::? It's really hard to read this way. Using the Table alias from abovem you could write
(ColumnInsertValue::Expression(Version,
AsExpression::<
<<Version as Expression>::SqlType as IntoNullable>::Nullable,
>::as_expression(version)
),)There was a problem hiding this comment.
I'm a big fan of type aliases btw. They can be used to make nice "reverse" associated type accessors like this (IIRC):
type SqlType<T> = <T as Expression>::SqlType;
type Nullable<T> = <T as IntoNullable>::Nullable;
diesel_codegen/src/insertable.rs
Outdated
| ColumnInsertValue::Expression( | ||
| #column, | ||
| AsExpression::<<<#column as Expression>::SqlType | ||
| as IntoNullable>::Nullable>::as_expression(#field_access) |
There was a problem hiding this comment.
Pls don't break at as. See above.
diesel_codegen/src/insertable.rs
Outdated
| } | ||
| ) | ||
| })) | ||
| .build(); |
There was a problem hiding this comment.
Can you use block ident? I'd write this as
…
.with_predicates(
model.generics.lifetimes
.iter()
.map(|l| {
syn::WherePredicate::RegionPredicate(
syn::WhereRegionPredicate {
lifetime: l.lifetime.clone(),
bounds: vec![insert.lifetime.clone()],
}
)
})
)
.build();(Also note the space before { and trailing comma in WhereRegionPredicate)
diesel_codegen/src/insertable.rs
Outdated
| #table_name::table, | ||
| Self, | ||
| Op, | ||
| Ret |
diesel_codegen/src/insertable.rs
Outdated
| fn make_column_insert_value( | ||
| a: &Attr, | ||
| table_name: &syn::Ident, | ||
| struct_name: &str |
diesel_codegen/tests/insertable.rs
Outdated
|
|
||
| #[test] | ||
| fn simple_struct_definition() { | ||
|
|
weiznich
force-pushed
the
remove_insertable_macro
branch
2 times, most recently
from
7e3e3b6
to
c5b6eae
Compare
weiznich
force-pushed
the
remove_insertable_macro
branch
from
c5b6eae
to
8832e24
Compare
|
Any news on this? |
|
I'd like to postpone these for the time being. I think there are two steps I'd like to take as separate PRs before we take this step:
That would make our job of reviewing significantly easier, help point out gaps in our test coverage, and make me more comfortable with this step. |
No description provided.