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

Reduce usage of String in translations code. #524

Merged
merged 7 commits into from
Jun 6, 2024
Merged

Reduce usage of String in translations code. #524

merged 7 commits into from
Jun 6, 2024

Conversation

CosminPerRam
Copy link
Contributor

This is a part of #523 (the non-lint part of it).
This PR removes an unnecessary to_string call on an already String value and replaces per-item .to_string() with an entire match block .to_string().

@GyulyVGC
Copy link
Owner

GyulyVGC commented Apr 29, 2024

Thanks!

While you are at it, would you mind updating all the translations methods to return an &'static str?

I think there are some cases where I actually don't need them to be strings, so this should be more efficient.

In cases Strings are actually needed, it'd be nice updating the code where the function is invoked calling .to_string() and leaving &str as return value of the method.

@GyulyVGC GyulyVGC added the enhancement New feature, request, or improvement label Apr 29, 2024
@GyulyVGC GyulyVGC added this to the v1.3.1 milestone Apr 29, 2024
@CosminPerRam CosminPerRam changed the title Remove some unnecessary to_string calls. Reduce usage of String in translations code. Apr 29, 2024
@CosminPerRam
Copy link
Contributor Author

CosminPerRam commented Apr 30, 2024

Although there is a mix usage of &'static str, String and Text<'static, StyleType>, I ought here to replace only the non-parameter-composing String instances as replacing Text ones would require bigger changes (that can be done in another PR).

@GyulyVGC
Copy link
Owner

Although there is a mix usage of &'static str, String and Text<'static, StyleType>, I ought here to replace only the non-parameter-composing String instances as replacing Text ones would require bigger changes (that can be done in another PR).

I agree, and I'm fine with this.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 6, 2024

@all-contributors please add @CosminPerRam for code.

Copy link
Contributor

@GyulyVGC

I've put up a pull request to add @CosminPerRam! 🎉

@GyulyVGC GyulyVGC merged commit d54c77f into GyulyVGC:main Jun 6, 2024
3 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature, request, or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants