Skip to content

Added ToStringFastLowerCase #39

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

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

Conversation

imperugo
Copy link

Just added a method that allow to have the lowercase of the enum name.

Not sure about the tests because my local built created weird trailing space that make unable to run the test locally, but the code looks ok.

Let me know if is ok, or need to change something.

Thanks

@andrewlock
Copy link
Owner

Hi @imperugo, thanks for this. My main concern is what the use case is for this 🤔 I assume it's for doing case-insensitive comparisons? If that's the case, I'd argue that StringComparison.OrdinalIgnoreCase provides a preferable API. Additionally, assuming that is the use case, it's better to use uppercase.

@imperugo
Copy link
Author

Hi @andrewlock
the reason is not related to comparing stuff.
Sometimes you need to generate strings for external systems that are using lowercase like storages, legacy systems and so on but internally you have the enum in this way (correctly from my point of view)

public enum EntityType{
   User = 0,
   Customer = 1,
   Product = 2
}

if you have to create a string like this /dev/user/user-id/.... you have to do that /dev/{entityType.ToFastString().ToLowerCaseInvariant()/user-id/.....

With this PR you can save the cost of allocating a string doing this /dev/{entityType.ToFastStringLowerCase()/user-id/.....

Thanks

@andrewlock
Copy link
Owner

Thanks for the context @imperugo 🙂

Have you considered using the existing [DisplayName] support? e.g. if you defined your enum like this:

public enum EntityType{
   [DisplayName("user")] User = 0,
   [DisplayName("customer")] Customer = 1,
   [DisplayName("product")] Product = 2
}

Then this would already work with ToStringFast().

The scenario you describe is certainly valid, but I'm hesitant to add a ToFastStringLowerCase() method as it's just not an API you typically see. For example the same scenario may require uppercase, or to use the current culture, etc, should we support all those too? We could add a ToStringFast that takes a culture + upper/vs lower overloads etc, but that feels excessive IMO.

@imperugo
Copy link
Author

imperugo commented Sep 20, 2022

What does it mean "it's just not an API you typically see"?
Also ToFastString is not. Doesn't it?

Other than that I agree with Upper and Lower as parameters of ToFastString but I didn't understand how do you think to handle the cultures.
Would you like to add all possible culture combination?
Is there a scenario where the culture can change the string output of a enum name?

@andrewlock
Copy link
Owner

What does it mean "it's just not an API you typically see"?

I mean, you won't see ToStringLowerCase anywhere in a standard library, other than on String itself, it's an unusual API to add to a type, which is why I'm hesitant to add it.

Also ToFastString is not. Doesn't it?

Absolutely, it's unusual, ideally it would just be ToString(), but you know, that's taken 😄

Other than that I agree with Upper and Lower as parameters of ToFastString but I didn't understand > how do you think to handle the cultures.
Would you like to add all possible culture combination?

My point really is that special-casing Lower isn't something I'm sure we want to do. There's nothing special about lower that would mean we shouldn't also do upper etc etc, and given the [DisplayName] option I don't know if it's an API we should add.

Is there a scenario where the culture can change the string output of a enum name?

Yes, I believe so, a classic example is the Turkish I problem

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