Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change a few method signatures #117

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

somedevfox
Copy link
Contributor

@somedevfox somedevfox commented Mar 29, 2023

Summary

Change a few method signatures to accept impl Into<String>, instead of &str

Motivation

Currently, several methods like MessageDialog::set_title, MessageDialog::set_description or FileDialog::add_filter accept &str as one of the arguments - this is extremely inconvenient, as if you have a String from format! macro, one has to convert it to &str and sometimes it may not be possible, due to the borrow checker.
And internally - titles, descriptions, extensions, etc. are stored as Strings, so it'd make more sense to convert anything to a String in the methods themselves.

Why Into<String> and not ToString?

To tackle this question, we first need to know the difference between two traits.

ToString - converts the value to String without consuming it.
Into<String> - consumes the value while converting it to a String.

All methods in this PR use the arguments to show data in message boxes - therefore it's most unlikely for that data to be used after the message box is shown.
If one really needs to use the argument after a method from rfd, one can clone or copy the value.

Compatibility with existing codebases

This PR doesn't require any overhaul for the users of the library, as &str implements both Into<String> and ToString

@PolyMeilex PolyMeilex merged commit c285eea into PolyMeilex:master Apr 26, 2023
@PolyMeilex
Copy link
Owner

Thanks!

@somedevfox
Copy link
Contributor Author

<3

@somedevfox somedevfox deleted the change-method-signatures branch April 26, 2023 20:00
@repi repi mentioned this pull request Aug 29, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants