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

Some small refactorings #2305

Merged
merged 2 commits into from
Feb 20, 2020
Merged

Some small refactorings #2305

merged 2 commits into from
Feb 20, 2020

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Feb 18, 2020

I first thought I couldn't remove use diesel; because some crates didn't have edition = "2018" and started working on that. Then I realized the code the macros generate have to work in the 2015 edition so I probably can't remove the redundant import. (I still don't understand why things break inside diesel when I remove it, but that's not that important)

If you want me to split this PR or remove the first commit altogether, I'm fine with that.

@weiznich weiznich requested a review from a team February 18, 2020 19:19
@weiznich
Copy link
Member

I would prefer to have the 2018 edition change in a different PR.

Then I realized the code the macros generate have to work in the 2015 edition so I probably can't remove the redundant import. (I still don't understand why things break inside diesel when I remove it, but that's not that important)

So the problem here is the following: We use the macros also in diesel itself, as they are useful for us too. For example sql_function! is quite widely used internally to define various sql functions. In the end all of those macros generate some code that relies on types from diesel being scope. That needs to be wrapped somehow by the macro, as we cannot assume that the user will import all of those types (Some of them are even not considered part of the public API). That implies we need something to import from. For cases where someone else uses diesel we just assume that there exists a crate named diesel that we can import. That use diesel; brings it into local scope. For our internal use things are a bit more complicated. We decided to workaround that by just having a mod diesel { pub use super::*;'} in our crate root. That allows proc macros to refer to diesel as like it was an external crate even inside diesel itself. In the end it's just a hack to workaround that there is nothing like $crate for proc macros.

@jplatte
Copy link
Contributor Author

jplatte commented Feb 18, 2020

Interesting! Should I add a comment to the use diesel; import that explains this?

@weiznich
Copy link
Member

That's a good idea, otherwise I've forgotten that the next time I look at that code 🙈

@jplatte
Copy link
Contributor Author

jplatte commented Feb 19, 2020

So apparently CI does cargo fmt on 1.36.0 while you have a rust-toolchain file with 1.37.0? Not that it makes a difference here (they produce the same output, some later version changes things up in one place), I had a local rustup override to see newer clippy lints, but it does confuse me a bit.

@jplatte
Copy link
Contributor Author

jplatte commented Feb 19, 2020

CI failure seems unrelated.

@weiznich weiznich merged commit 63b404c into diesel-rs:master Feb 20, 2020
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 this pull request may close these issues.

4 participants