dsync icon indicating copy to clipboard operation
dsync copied to clipboard

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

Open jjangga0214 opened this issue 2 years ago • 9 comments

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.

jjangga0214 avatar Oct 16 '23 13:10 jjangga0214

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)

hasezoey avatar Oct 16 '23 15:10 hasezoey

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 :)

Wulf avatar Oct 28 '23 21:10 Wulf

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 avatar Nov 06 '23 16:11 hasezoey

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?

hasezoey avatar Nov 19 '23 13:11 hasezoey

IMHO, the latter is better :)

jjangga0214 avatar Nov 19 '23 13:11 jjangga0214

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)

Wulf avatar Dec 10 '23 18:12 Wulf

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

hasezoey avatar Dec 11 '23 16:12 hasezoey

@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| {

longsleep avatar Feb 11 '24 18:02 longsleep

@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.

hasezoey avatar Feb 12 '24 15:02 hasezoey