Skip to content

Conversation

hitbear
Copy link

@hitbear hitbear commented Feb 9, 2024

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.

…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.
Copy link
Collaborator

@emilio emilio left a 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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems drive-by?

Copy link
Author

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.

Copy link
Collaborator

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.

Copy link
Author

@hitbear hitbear Feb 27, 2024

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;)

Copy link
Collaborator

@emilio emilio left a 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!

out.new_line_if_not_start();
match self.config.language {
Language::C | Language::Cxx => {
write!(out, "/* Package version: {} */", self.package_version,);
Copy link
Collaborator

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.

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 {
Copy link
Collaborator

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 {
Copy link
Collaborator

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.

Copy link
Author

@hitbear hitbear Feb 27, 2024

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]
Copy link
Collaborator

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?

Copy link
Author

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"]

Copy link
Collaborator

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"))]
Copy link
Collaborator

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?

Copy link
Author

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.

emilio added a commit that referenced this pull request Apr 14, 2024
…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>
emilio added a commit that referenced this pull request Apr 14, 2024
…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>
github-merge-queue bot pushed a commit that referenced this pull request Apr 14, 2024
…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>
@emilio emilio closed this in #939 Apr 14, 2024
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.

2 participants