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

feat: Also return the interface name #11

Merged
merged 23 commits into from
Sep 4, 2024
Merged

feat: Also return the interface name #11

merged 23 commits into from
Sep 4, 2024

Conversation

larseggert
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.24%. Comparing base (655a937) to head (6084033).
Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
src/lib.rs 89.18% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
+ Coverage   93.05%   93.24%   +0.18%     
==========================================
  Files           1        1              
  Lines         144      222      +78     
==========================================
+ Hits          134      207      +73     
- Misses         10       15       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@larseggert larseggert marked this pull request as ready for review September 3, 2024 07:20
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@larseggert larseggert changed the title feat: Also return an interface ID feat: Also return the interface name Sep 3, 2024
@larseggert
Copy link
Collaborator Author

larseggert commented Sep 4, 2024

@djc so how do I now get updated Windows bindings when I need to add support for a new function? I don't have access to a Windows machine to run bindgen.

@djc
Copy link
Member

djc commented Sep 4, 2024

@djc so how do I now get updated Windows bindings when I need to add support for a new function? I don't have access to a Windows machine to run bindgen.

I did it via CI, which will print the newly generated bindings when the comparison fails -- but you have to get the integration test to run, which only happens when the library compiles. Might be an option to put the codegen in a separate crate to make that easier...

@larseggert
Copy link
Collaborator Author

I did it via CI, which will print the newly generated bindings when the comparison fails -- but you have to get the integration test to run, which only happens when the library compiles. Might be an option to put the codegen in a separate crate to make that easier...

Hm. This is getting complex fast. I'm tempted to revert #14 and just go back to what we had before...

@djc
Copy link
Member

djc commented Sep 4, 2024

If you just make a separate branch/PR that adds the newly required symbols only in the codegen test, that should make things easier.

@larseggert
Copy link
Collaborator Author

larseggert commented Sep 4, 2024

but you have to get the integration test to run

Could we run bindgen in build.rs? IIRC @martinthomson suggested something similar.

@djc
Copy link
Member

djc commented Sep 4, 2024

but you have to get the integration test to run

Could we run bindgen in build.rs? IIRC @martinthomson suggested something similar.

Like the previous approach, that optimizes for the maintainers at the expense of every downstream build ever, which doesn't seem like a good trade-off to me.

@larseggert
Copy link
Collaborator Author

The "separate PR" thing worked, thanks!

Copy link
Collaborator

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks!

Definitely in favor of returning a UTF-8 String over a hash.

src/lib.rs Show resolved Hide resolved
@larseggert larseggert merged commit 3ee7d47 into main Sep 4, 2024
16 of 17 checks passed
@larseggert larseggert deleted the feat-iface-id branch September 4, 2024 11:14
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