search

Electron100 / butane

An ORM for Rust with a focus on simplicity and on writing Rust, not SQL
Apache License 2.0
91 stars 13 forks source link

More flexibility in accepting DateTime identifiers #54

Closed Electron100 closed 1 year ago

Electron100 commented 1 year ago

Allows chrono::DateTime alongside DateTime Fixes #53

The real issue here is that the migration type resolution is relying on the proc macro, which sees only the AST and not the type system. So when it sees a type identifier, it sees the path segments that are in the AST, not the actual type, meaning that chrono::DateTime and DateTime (after use chrono::DateTime) are not the same! The latter was getting accepted and the former is not.

This PR is a bandaid to discard the path information and just look at the last segment (hoping it was imported from chrono!) to make this slightly more flexible.

This type of issue is the biggest reason I want to move migration generation from proc-macro time to something triggered by the butane cli command.

jayvdb commented 1 year ago

I'll test this on my project which is using pub some_date: Option<chrono::DateTime<chrono::offset::Utc>>,

In order to verify that these type of changes work with migration system, it would be useful to use the type in the example/ project, which now has tests that run makemigration.

Electron100 commented 1 year ago

In order to verify that these type of changes work with migration system, it would be useful to use the type in the example/ project,

The tests under butane/tests are fit for this purpose as well. The test setup in setup_db from butane/tests/common/mod.rs creates a migration, and the change this PR made to butane/tests/basic.rs was sufficient to trigger the failure prior to the fix. The tests did previously include a DateTime, with a previous use chrono::DateTime but not the qualified-in-place chrono::DateTime. While these are semantically the same, they aren't the same in the AST the proc macro sees.