-
Notifications
You must be signed in to change notification settings - Fork 351
Added support for integrating the package_version #926
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
Conversation
…mment of the header file. The new command line argument for this is --package-version. The TOML config file flag is package_version=true||false.
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.
Looks fine, but needs a test, and probably undoing the rust-toolchain change. Let me know if you need any help with that.
@@ -1,2 +0,0 @@ | |||
[toolchain] | |||
channel = "nightly" |
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.
This seems drive-by?
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.
Thank you for your response. Yes I fixed both requests but I think that I need some help to write an appropriate test.
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.
You can add files to tests/rust
where your toml file has the new option.
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.
Ok, in my latest commit I added a test crate package_version that checks whether the file is generated with the correct package version.
Since some other tests failed with cython, I modified cbindgen. Now comments such as header, trailer, etc. are written to the file after a check if the comment should be in C/C++ syntax (/*) or Python syntax (''').
cargo test
now tells ok. 145 passed;
And I also run
cargo fmt
;)
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.
Thanks! I think we should keep user inputs as-is and not mangle them, but the rest looks fine, thank you!
src/bindgen/bindings.rs
Outdated
out.new_line_if_not_start(); | ||
match self.config.language { | ||
Language::C | Language::Cxx => { | ||
write!(out, "/* Package version: {} */", self.package_version,); |
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.
I'd remove the trailing comma here and below.
src/bindgen/bindings.rs
Outdated
out.new_line(); | ||
} | ||
if let Some(ref f) = self.config.autogen_warning { | ||
out.new_line_if_not_start(); | ||
write!(out, "{}", f); | ||
match self.config.language { |
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.
I think we should keep these as it was, this contents are controllable by the user so we shouldn't try to mangle them.
|
||
out.new_line(); | ||
} | ||
|
||
if let Some(ref f) = self.config.header { |
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.
Same here, this should probably remain as it was.
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.
So with these two commits, I removed the user input modification - and adjusted also the package_version test. Yes I see, it could be dangerous to change a users input.
@@ -0,0 +1,7 @@ | |||
package_version = true | |||
|
|||
[parse.expand] |
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.
Do you need to use expand for this to work?
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.
no, to be honest =)
[parse.expand] | ||
crates = ["package_version"] | ||
features = ["cbindgen"] | ||
|
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.
remove redundant newlines?
@@ -0,0 +1,8 @@ | |||
#[repr(C)] | |||
pub struct Foo { | |||
#[cfg(not(feature = "cbindgen"))] |
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.
This seems orthogonal to the feature you're testing?
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.
Yes, actually I could remove the whole stub code but in my latest commit, i removed just this line.
…mment of the header file. The new command line argument for this is --package-version. The TOML config file flag is package_version=true||false. Closes #926 Co-Authored-By: Emilio Cobos Álvarez <emilio@crisal.io>
…mment of the header file. The new command line argument for this is --package-version. The TOML config file flag is package_version=true||false. Closes #926 Co-Authored-By: Emilio Cobos Álvarez <emilio@crisal.io>
…mment of the header file. The new command line argument for this is --package-version. The TOML config file flag is package_version=true||false. Closes #926 Co-Authored-By: Emilio Cobos Álvarez <emilio@crisal.io>
The package version information appears in a comment of the header file. The new command line argument for this is --package-version. The TOML config file flag is package_version=true||false.
This new feature is helpful when you are working on a growing Rust library with new functions and structs. The package version entry in the header file appears as
"/Package version: {}/",
and gives a better overview which header file fits to which version of the library.