-
Notifications
You must be signed in to change notification settings - Fork 45
chore: allow generated file to be used with include!
#108
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
base: main
Are you sure you want to change the base?
Conversation
include!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the code generation to make generated files compatible with Rust's include! macro by removing inner attributes (#![...] and //! ...), which are not allowed due to a compiler limitation. Generated code is now written to OUT_DIR during the build process and included via include! in user code, following Rust best practices for build-time code generation.
Key changes:
- Generated code now omits all inner attributes and doc comments, moving lint suppressions to module-level
#[expect(...)]attributes - All generated code is written to
OUT_DIRinstead of the source tree - Removed the
allow_dead_codeconfiguration option - Updated error trait implementation from
std::error::Errortocore::error::Errorfor betterno_stdcompatibility
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| testing/can-messages/src/messages.rs | Entire generated file removed from source tree |
| testing/can-messages/src/lib.rs | Replaced static module with include! from OUT_DIR, added module-level lint suppressions |
| testing/can-messages/build.rs | Simplified to write generated code directly to OUT_DIR instead of src/ |
| testing/can-embedded/src/messages.rs | Stub file removed as it's now generated during build |
| testing/can-embedded/src/lib.rs | Added include! macro to load generated code from OUT_DIR |
| testing/can-embedded/build.rs | New build script to generate messages.rs for embedded target |
| testing/can-embedded/Cargo.toml | Added build dependencies for code generation |
| src/lib.rs | Removed inner attributes and doc comments from generated code, changed error trait to core::error::Error, removed allow_dead_code config |
| README.md | Removed reference to allow_dead_code configuration option |
| Cargo.lock | Dependency version updates |
| .github/workflows/ci.yml | Updated cargo commands to use --workspace instead of --all |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
61b4663 to
2a186df
Compare
tegimeki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me and address issues I've considered important, namely having generated code in OUT_DIR rather than within the source tree. However, not being a user of this crate currently it seems best to ping some of the recent committers to get their take: @killercup @scootermon @projectgus @trnila @cpctaylo
This seems like a breaking change, not necessarily at the API level but in terms of how dependent projects might use the generated files... what kind of version bump is anticipated?
| //.impl_arbitrary(FeatureConfig::Gated("arbitrary")) // Optional impls. | ||
| //.impl_debug(FeatureConfig::Always) // See rustdoc for more, | ||
| //.check_ranges(FeatureConfig::Never) // or look below for an example. | ||
| .build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add imports to have working example in README?
use dbc_codegen::Config;
use std::fs::write;
use std::{env::var, path::PathBuf};dbc_file should be as a string now:
let dbc_file = std::fs::read_to_string(dbc_path).unwrap();There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trnila I made README to be the only documentation for the crate, and for it to be auto-compiled as part of the doc tests.
* Due to a compiler limitation, inner attributes are not allowed with the `include!` macro. This PR removes all `#![...]` and `//! ...` from the generated code, which will result in some warnings being generated. We may need to add explicit `allow` to each individual function where relevant. * Do not place generated code inside the user's code - not a good practice. All generated code should go into `OUT_DIR`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 14 out of 17 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
include!macro. This PR removes all#![...]and//! ...from the generated code, and adds many explicitallowto individual functions and use statements where relevant.OUT_DIR.allow_dead_codeconfiguration