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.

Sign up for GitHub

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

Jump to bottom

Add OsString support to windows-registry crate #3121

Closed
wants to merge 1 commit into from
Closed

Add OsString support to windows-registry crate #3121

wants to merge 1 commit into from

Conversation

kennykerr
Copy link
Collaborator

Fixes: #3119

@kennykerr
Copy link
Collaborator Author

Wow, that's quite the bizarre nightly behavior. 🤪

image

@kennykerr
Copy link
Collaborator Author

kennykerr commented Jun 24, 2024 •
edited
Loading

Looks like that's a rustfmt bug:

rust-lang/rustfmt#6210
rust-lang/rust#126888

@kennykerr kennykerr requested a review from ChrisDenton June 24, 2024 14:04
@kennykerr kennykerr mentioned this pull request Jun 24, 2024
Closed
ChrisDenton
ChrisDenton approved these changes Jun 24, 2024
View reviewed changes
Copy link
Collaborator

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

Looks good to me. Seems like the rustfmt bug will fix itself in the next nightly.

@ChrisDenton
Copy link
Collaborator

Oh it just occurred to me that maybe we could just return a HSTRING or a Vec<u16> and let users turn that into a OsString or whatever they want. It's less ergonomic for the OsString case but more flexible in general.

@kennykerr
Copy link
Collaborator Author

Yes, I thought about using HSTRING instead. It's way better and provides convertibility with OsString. I just need to pull HSTRING and friends into a new windows-strings crate before I can add a dependency from windows-registry. I need to do that anyway, so perhaps I'll wait on this PR and we can just use HSTRING here.

@ChrisDenton
Copy link
Collaborator

To be honest, that does sound a lot more appealing to me now that I've considered it some more.

@kennykerr kennykerr closed this Jun 24, 2024
This was referenced Jun 25, 2024
Merged
Merged
@kennykerr kennykerr deleted the registry-os-str branch August 1, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisDenton ChrisDenton ChrisDenton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should windows_registry::Key::(get|set)_string use OsStr instead of str?
2 participants
@kennykerr @ChrisDenton