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

Use OsString for System #1230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented Mar 19, 2024

This uses OsString for the methods on System.

This uses `OsString` for the methods on `System`.
@CryZe
Copy link
Contributor Author

CryZe commented Mar 19, 2024

This is where maybe some of these start to not necessarily make sense. It's just one of N branches I created back then after switching everything.

@GuillaumeGomez
Copy link
Owner

Indeed. What's your logic backing this change? I don't expect any of these cases to ever handle non-utf8 strings.

@CryZe
Copy link
Contributor Author

CryZe commented Mar 23, 2024

There's no logic, I just did a full conversion of the entire crate (for fun? no idea, don't remember). We can close this and instead discuss which APIs would benefit from it, so I can open those PRs (or none at all). Alternatively maybe it can be similar to std::env::args / std::env::args_os. Where internally it's all managed as OsString, but there's generally convenience methods for when you really expect UTF-8 strings.

@GuillaumeGomez
Copy link
Owner

At least in this case, the strings are not meant to be reused in sysinfo API, so that seems more complex than necessary as we tend to use String and not OsString in general.

I think "if it's used in sysinfo API" is a good metric to talk whether or not an API should use OsString or not. What do you think?

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.

None yet

2 participants