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

Avoid extra clone in config if possible #1137

Merged
merged 1 commit into from
Jul 6, 2024
Merged

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented May 2, 2024

Using impl Into<String> instead of &str in a fn arg allows both &str and String as parameters - thus if the caller already has a String object that it doesn't need, it can pass it in without extra cloning.

The same might be done with the password, but may require closer look.

Using `impl Into<String>` instead of `&str` in a fn arg allows both `&str` and `String` as parameters - thus if the caller already has a String object that it doesn't need, it can pass it in without extra cloning.

The same might be done with the password, but may require closer look.
@sfackler
Copy link
Owner

sfackler commented May 2, 2024

I'm not super opposed to this, but at the same time I'm not sure it'll make a huge difference. What portion of runtime is spent constructing config?

@nyurik
Copy link
Contributor Author

nyurik commented May 2, 2024

@sfackler config specifically is not that big of a deal of course. My concern is that any frequently used code teaches users on how to do things - and in this case, IMO, the API should have only allowed String as the parameter, but because breaking API is not worth it, might as well introduce a backwards compatible solution.

Reasoning:

P.S. community recommended using explicit generic rather than impl Into<String> as a param type.

@sfackler sfackler merged commit ded5e7d into sfackler:master Jul 6, 2024
@nyurik nyurik deleted the use-string branch July 7, 2024 12:23
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