| vandenoever |
After enabling 2018 edition on my cargo project, the use statements
generated in the interface module no longer resolve.
Prefixing module paths with crate:: brings things back into alignment.
Unsure if this is how you'd like to approach the issue, but I'm happy to revise the patch based on your feedback.
STEPS TO REPRODUCE
OBSERVED RESULT
Build fails since generated code tries to import crate-local modules as if they are top level crates.
EXPECTED RESULT
Build completes successfully.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Thank you for the simple patch. Is use-2018-edition a feature that is automatically set by Cargo when edition = "2018"? I've not found the documentation on that.
I've tried the patch. To get a 2018 project to compile, cargo needs to know about the 2018 edition feature:
rust_qt_binding_generator = { path = "../rust_qt_binding_generator", features = ["use-2018-edition"] }Needing to set the feature is a burden on the developer. Can it be avoided?
I would prefer it if the patch would parse Cargo.toml to determine the edition of Rust. This can be done with the toml crate.
You figured it all out it seems. You are correct - use-2018-edition is a custom crate feature which cargo doesn't know anything about. When the editions mechanism was announced, I was surprised to find there was no cfg support for introspecting the edition set by the crate, so I thought the most direct path would be to set this up as a feature and allow the user to opt-in manually.
Parsing Cargo.toml should be easy to do, however. I'll revise the patch shortly.
Initially, I considered there may have been a use case for running on the CLI versus as a build dependency where we might not know precisely which Cargo.toml to examine, but I think there are already a number of assumptions made about the layout of the project so one more probably isn't going to hurt too much.
Thanks for your consideration!
Dropped the feature and replaced it with parsing the target crate's manifest to read the edition field. The value of the edition field is being attached to the Config struct (top level) since it felt a little awkward to include it with the "rust" section of the struct which is parsed separately from the bindings.json.
I elected to match 2018 exactly (not using >=, for example) since it will surely be a long time before we see another edition, and who knows what may come between now and then. I'd imagine if there are other significant edition changes in the future, we will need to inspect more of the manifest, etc. Combine that with the fact I don't think there are any assurances that editions will all be named for YYYY, I'm not sure I like being overly broad.
It's a very clear patch and well documented. I'd prefer it to use an enum though. I've put a suggestion in the code.
The patch works for my projects.
If you want to change to use the enum, please update. If not, say so and I'll accept and commit this version and then do the change to use an enum.
| src/configuration.rs | ||
|---|---|---|
| 66 | How about using an enum here? pub enum RustEdition {
Rust2015,
Rust2018,
Unknown
}
impl std::convert::From<Option<&str>> for RustEdition {
fn from(str: Option<&str>) -> RustEdition {
match str {
None | Some("2015") => RustEdition::Rust2015,
Some("2018") => RustEdition::Rust2018,
_ => RustEdition::Unknown,
}
}
} | |
| 458 | With an enum the code avoids the unwrap. let rust_edition: RustEdition = {
...
manifest["package"].get("edition").and_then(|s| s.as_str()).into()
}; | |
| src/rust.rs | ||
| 1541 | match conf.rust_edition {
RustEdition::Rust2018 => "crate::",
_ => "", | |
Updated to use enums to represent edition versions.
Good feedback! This looks much more friendly to not have to match on Option<String> which requires awkward juggling. It also removes the temptation to rely on ordering when comparing editions.
Thanks for talking me through how you'd want this feature implemented, and yes, this is a great project which is opening some interesting doors for us.