Wulf / dsync

Generate rust structs & query functions from diesel schema files
Other
70 stars 13 forks source link

Change `use crate::diesel::*;` import to be more specific and not require `crate::` #94

Open jjangga0214 opened 1 year ago

jjangga0214 commented 1 year ago

https://github.com/Wulf/dsync/blob/007ace83c139f67e1d85ed4829cf0b6953d121df/test/simple_table/models/todos/generated.rs#L3

dsync generates use crate::diesel::*;.

But I think it actually should be use diesel::*;.

diesel is an external crate, which is not declared from the root of a user's project.

hasezoey commented 1 year ago

i guess that is true, but currently it is somewhat expected you do a pub use diesel; in the entry-point of your crate (like lib.rs)

Wulf commented 11 months ago

Good catch @jjangga0214! @hasezoey is right, we often include extern crate diesel or a use statement in our entry points. I think we can safely remove the crate:: prefix :)

hasezoey commented 11 months ago

Re-opening the issue because of #116, this will need a better solution than just replacing the wildcard import (with some more specific import or a alias)

hasezoey commented 11 months ago

i am thinking of fixing this, but there are 2 ways:

1: with a alias (if yes, what alias)

use diesel as D;

#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, D::Queryable, D::Selectable, D::QueryableByName)]
#[D::diesel(table_name=todos, primary_key(id))]
pub struct Todos {
    pub id: i32,
    pub unsigned: u32,
    pub unsigned_nullable: Option<u32>,
    pub text: String,
    pub completed: bool,
    pub type_: String,
    pub created_at: chrono::DateTime<chrono::Utc>,
    pub updated_at: chrono::DateTime<chrono::Utc>,
}

or 2: directly:

#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::Queryable, diesel::Selectable, diesel::QueryableByName)]
#[diesel::diesel(table_name=todos, primary_key(id))]
pub struct Todos {
    pub id: i32,
    pub unsigned: u32,
    pub unsigned_nullable: Option<u32>,
    pub text: String,
    pub completed: bool,
    pub type_: String,
    pub created_at: chrono::DateTime<chrono::Utc>,
    pub updated_at: chrono::DateTime<chrono::Utc>,
}

which would be better?

(using direct imports like use diesel::{Queryable} is not possible because that may conflict with user names and using use diesel::{Queryable as DQueryable} or similar is quite a lot of clutter than the other options)

i personally think using diesel:: everytime adds quite the clutter

@Wulf what do you think would be better?

jjangga0214 commented 11 months ago

IMHO, the latter is better :)

Wulf commented 10 months ago

hey @hasezoey thanks for investigating and thinking of solutions. Again, sorry for the late reply, I've been in the middle of a move!

Let's go with diesel::, it's copy-pastable and less ambiguous. I think it'll make the dsync code easier to maintain as well (we won't need to manage aliasing)

hasezoey commented 10 months ago

created #122 which changes most things to use the diesel:: prefix, but this does not completely solve this issue yet because of the diesel trait imports, where i am not sure which are fully required and would like to wait for #114 and add additional tests as pointed out by #119

PS: also changed the issue title to better reflect what this is about

longsleep commented 8 months ago

@hasezoey the changes in https://github.com/Wulf/dsync/pull/114 result in compile time warning warning: unused import:crate::diesel::*`` since notning is used from there anymore and hence that generated line should be removed.

Update: removing this makes the advanced_queries tests fail. So i guess it is best to pull in the diesel::prelude::* globally but allow it to be unused.

I suggest a fix something like this

diff --git a/src/code.rs b/src/code.rs
index 3ae6224..113bdea 100644
--- a/src/code.rs
+++ b/src/code.rs
@@ -731,7 +731,11 @@ fn build_imports(table: &ParsedTableMacro, config: &GenerationConfig) -> String
     // Note: i guess this could also just be a string that is appended to, or a vec of "Cow", but i personally think this is the most use-able
     // because you dont have to think of any context style (like forgetting to put "\n" before / after something)
     let mut imports_vec = Vec::with_capacity(10);
-    imports_vec.push("use crate::diesel::*;".into());
+    imports_vec.extend([
+        "#[allow(unused)]".into(),
+        "use diesel::prelude::*;".into(),
+        "".into(),
+    ]);

     let table_options = config.table(&table.name.to_string());
     imports_vec.extend(table.foreign_keys.iter().map(|fk| {
hasezoey commented 8 months ago

@longsleep known issue, i just ignored it for now to actually get to removing the line completely and use traits with absolute paths, but i wanted #114 merged first to make it easier to find the issues and test the methods actually compile. though yes, i guess a quick fix would be applicable.