-
Notifications
You must be signed in to change notification settings - Fork 893
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
Replace winreg
with windows-registry
#3896
Conversation
windows-registry
winreg
with windows-registry
a6c903b
to
ea9c564
Compare
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 haven't done a complete review yet, because I think there's a bunch of stuff left to clean up. The core change looks great though, thanks for working on this!
c061ec6
to
605b3c4
Compare
5b6be5c
to
811c141
Compare
71b9a16
to
4c3ff9c
Compare
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.
Overall a really nice improvement!
5fa045c
to
4c3ff9c
Compare
Sorry, but it turns out I had to make some changes to registry-related API in #3893. Hopefully it won't be too hard to rebase on top of my changes. |
Oh, I think I'd better replace dependency from scratch lest appear some unexpect problems.😅 |
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 PR is getting even clearer than the previous iteration, modulo some eventual rebases (esp. after @kennykerr's possible API adjustments). Good job!
PS: As a favor, please consider listing @ChrisDenton as a co-author of some (or all) of your commits when applicable :)
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 looks good to me, thanks for all the work!
I could implement Deref in place of as_wide and that would provide all of this directly. |
Current status: Waiting for a new upstream release to ship microsoft/windows-rs#3148... |
Here's the |
2a8f0a4
to
00db126
Compare
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.
@kennykerr what's the rationale for using all-uppercase names for stuff like HSTRING
and HRESULT
? Are you trying to match the upstream names exactly? It looks rather unidiomatic.
There are countless identifiers in the Windows API. It's just not very practical to come up with different names - it has been attempted. It also makes it a lot harder to cross-reference documentation and source code for equivalent types. |
What about these changes? |
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 this is looking great. @InfyniteHeap and @kennykerr thanks for all the work! Would be nice to get that windows-registry release out so that we can move forward with this.
Coming up: microsoft/windows-rs#3293 |
Version 0.3.0 of the windows-registry crate has been published. https://github.com/microsoft/windows-rs/releases/tag/0.60.0 |
f716960
to
0bbcad0
Compare
0bbcad0
to
2f0399a
Compare
Can you squash all of your changes into a single commit? |
I've done this. |
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.
LGTM, and thanks for your work @InfyniteHeap!
PS: You may proceed with cleanup PRs such as the one discussed in #3896 (comment) shortly after.
Closes #3779.
APIs provided by
winreg
crate is not safe, simple and abstract enough. Besides that, it probably also contains protential bugs. Because of that, it is better to replace it withwindows-registry
crate, which is officially published and maintained by Microsoft.