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

v2 - type upgrade from string to flag enum - easy catch for consumers #1924

Open
baywet opened this issue Nov 13, 2024 · 3 comments
Open

v2 - type upgrade from string to flag enum - easy catch for consumers #1924

baywet opened this issue Nov 13, 2024 · 3 comments
Assignees
Milestone

Comments

@baywet
Copy link
Member

baywet commented Nov 13, 2024

While I support the use of a flaggable enum for performance reasons, I believe this is going to lead to a regression that's going to be hard to catch for most people. And which I think our own code is a victim of.

v1 Type is a string
v2 Type is a flaggable enum (currently)

It's super easy to not notice the change in some scenarios, especially the ones involving string interpolation, implicit operators. In that case, overlooking the fact that multiple values are present will lead to cases with multiple entries falling off a cliff.

I think to start with, our to identifier method should throw if multiple values are present, instead of returning null to signal something was wrong in the logic of the application

@darrelmiller darrelmiller self-assigned this Nov 14, 2024
@darrelmiller
Copy link
Member

@baywet We should talk.

@darrelmiller
Copy link
Member

@baywet After discussing this with @MaggieKimani1 we were wondering if it would make sense to change the signature of ToIdentifier to return an array of strings and the caller can deal with simplifying the output to a simple string if the array length is one.

@baywet
Copy link
Member Author

baywet commented Nov 19, 2024

I think it's be a step forward, however it doesn't solve the string interporlation or implicit operator issue.

If today I'm doing

if (schema.Type == "string")
{
 // do something
}

I believe this will still compile with v2, but depending on the description, you might end up with string,number as a value at runtime, derailing the application logic.

@darrelmiller darrelmiller added this to the NET:2.0 milestone Nov 26, 2024
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

No branches or pull requests

2 participants