Skip to content
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

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

Open
jjangga0214 opened this issue Oct 16, 2023 · 9 comments · Fixed by #109
Open

Comments

@jjangga0214
Copy link

jjangga0214 commented Oct 16, 2023

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
Copy link
Collaborator

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
Copy link
Owner

Wulf commented Oct 28, 2023

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
Copy link
Collaborator

hasezoey commented Nov 6, 2023

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
Copy link
Collaborator

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
Copy link
Author

IMHO, the latter is better :)

@Wulf
Copy link
Owner

Wulf commented Dec 10, 2023

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
Copy link
Collaborator

hasezoey commented Dec 11, 2023

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 hasezoey changed the title fix: use crate::diesel::*; vs use diesel::*; Change use crate::diesel::*; import to be more specific and not require crate:: Dec 11, 2023
@longsleep
Copy link
Collaborator

longsleep commented Feb 11, 2024

@hasezoey the changes in #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 added a commit to longsleep-io/dsync that referenced this issue Feb 11, 2024
The diesel::prelude members should always be non-conflicting with diesel
code and hence are safe to import globally. Based on the options used
when generating the may or may not be used by the resulting file and
hence the import is allowed to be unused.

Related: Wulf#94
@hasezoey
Copy link
Collaborator

@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 added a commit to hasezoey/dsync that referenced this issue Feb 12, 2024
this is a quick fix, and the import should be removed in the future altogether

re Wulf#94
hasezoey added a commit to hasezoey/dsync that referenced this issue Aug 21, 2024
this is a quick fix, and the import should be removed in the future altogether

re Wulf#94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants