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

Use $crate to refer to diesels macros #1956

Closed
wants to merge 3 commits into from

Conversation

mehcode
Copy link
Contributor

@mehcode mehcode commented Jan 15, 2019

Enables direct macro imports for diesel macros. Removes #[macro_use] from diesel_cli crate to demonstrate.

$crate:: is only legal to be used in Rust 1.30+ but as of 1.4 diesel has a minimum rust version of 1.31.

See rust-lang/rust#35896 (comment) for a good explanation of how to take advantage of the new macro import system.

Enables direct macro imports for diesel macros. Removes #[macro_use] from diesel_cli crate to demonstrate.
@weiznich
Copy link
Member

@diesel-rs/core I think we should discuss where we want to expose those macros exactly.
The following is just a mind dump:

  • I would expose all derives from diesel::derives and maybe re-export the most important ones like Queryable from diesel::prelude::*
  • Basically the same thing for macros

Thinks to bikeshed: Maybe expose every macro just from diesel::macros and skip the distinction between macro_rule macros and derives?

@mehcode
Copy link
Contributor Author

mehcode commented Jan 15, 2019

Good point. I didn't think about this.


From what I understand derive macros are not meant to be imported separately from their traits. On phone so I can't look it up but I remember a discussion about this where it was agreed importing them separately should be discouraged.

Remember that macros have a different namespace than traits.

// Import _both_ the derive macros and trait
use diesel::deserialize::Queryable;
// import derive macro and trait separately
use diesel::derives::Queryable;
use diesel::deserialize::Queryable;

@weiznich weiznich added this to the 2.0 milestone Jan 16, 2019
@mehcode
Copy link
Contributor Author

mehcode commented Jan 17, 2019

@weiznich Why 2.0? This isn't a breaking change. No matter where we move the macro re-exports, #[macro_use] usage will remain the same.

@weiznich
Copy link
Member

Sure that's no breaking change, but I do not see that we get to a consense here before we released 1.4. The next planed version after that will be 2.0 😉

@mehcode mehcode mentioned this pull request Jan 24, 2019
@mehcode
Copy link
Contributor Author

mehcode commented Jan 26, 2019

@sgrif @weiznich Any thoughts on how we want the macros?

My basic proposal would be:

  • Derive macros available when importing their respective traits (e.g., use diesel::deserialize::Queryable will import both the derive and the trait)
  • Other macros placed in related modules. The more I think on it, the more I dislike a generic diesel::macros bucket.

@sgrif
Copy link
Member

sgrif commented Jan 26, 2019

Why 2.0? This isn't a breaking change.

It's not, but this represents a pretty major change to the "normal" way to use Diesel, so I think it makes sense to include it as a major part of 2.0.

Any thoughts on how we want the macros?

Derives should definitely not be a thing the user ever has to think about. They should for sure be exported with the trait in question. Almost all of those are already in prelude, any that aren't probably should be. This is especially important for derives that don't map to an individual trait, such as #[derive(SqlType)].

As for the bang macros, I think this should probably be considered on a case by case basis. schema.rs currently does not include any imports. However, that file is usually generated and we can certainly add them. Especially considering that the macros used there (table!, allow_types_to_appear_in_same_query!, joinable!, etc) are unlikely to appear elsewhere in a program (unless it's our test suite or a plugin).

I'd be interested in seeing an audit of all our public bang macros, and seeing how many of them are likely to appear in non-generated files, how much we think they'll appear in app code vs plugin code, and where we might think to put them.

The more I think on it, the more I dislike a generic diesel::macros bucket.

Yeah, I think I agree with this. Glancing through the list of macros we export today (that I'm honestly not sure is complete because it's shorter than I remember), most of these have a vaguely clear place to put them. e.g. diesel_*_operator can be in diesel::expression (and those are probably only being used in library code). sql_function! is probably the only one that is worth putting in prelude I think. I'm not sure. I'd love to see some exploration here.

The only things I could see going into a macros bucket are the stuff called from schema.rs but we can probably call that schema if that ends up being the case.

@mehcode
Copy link
Contributor Author

mehcode commented Feb 9, 2019

Some bad news. Rust doesn't let you organize macro_rules macros. What that means is they all have to be at root. We could move out all the macros into a diesel-macros crate and then that should allow us to organize into modules via re-exports.


I did organize the derive() macros and re-exported them with their traits. I came across 3 that didn't map to a specific trait so I just left those being exported from diesel root.

@weiznich
Copy link
Member

Some bad news. Rust doesn't let you organize macro_rules macros. What that means is they all have to be at root. We could move out all the macros into a diesel-macros crate and then that should allow us to organize into modules via re-exports.

At least something like this seems ~work. Unfortunately I've found no way to hide the documentation output in the crate root, at least not in 5 minutes playing around with that. (Maybe that's not possible and we should just try to make it possible in rustdoc?) cc @sgrif about that.

mod macros {
    #[macro_export]
    macro_rules! foo {
        ($($tt:tt)*) => {}
    }
}


pub mod public_macro {
    #[doc(inline)]
    pub use super::foo;
}

@fbruetting
Copy link

fbruetting commented Mar 1, 2019

Would be cool if this could be merged. Diesel currently doesn’t align with the Rust book, so it is hard for beginners to grasp, why we need to use external crate, as this command is not introduced in the book anymore! By needing to place this into the crate root, Diesel also violates the privacy aspect of modules, described in the book.

@weiznich
Copy link
Member

weiznich commented Mar 4, 2019

@fbruetting This is planed for the next release, but it needs some work before it could be merged. Especially someone needs to figure out where exactly each macro should be exported (and make the documentation appear there).

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