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

change Message.type's type from String to enum? #38

Closed
uchijo opened this issue Nov 18, 2023 · 3 comments
Closed

change Message.type's type from String to enum? #38

uchijo opened this issue Nov 18, 2023 · 3 comments

Comments

@uchijo
Copy link
Contributor

uchijo commented Nov 18, 2023

As Message.type's value has only 7 candidates, it might be good to create enum, maybe.

here are pros and cons:

pros

  • better switch statement experience
    • enum enables exhaustive switch feature, which leads us less bugs
    • IDE's add missing switch cases feature will also be enabled with it
  • more readable
    • we can know what message is supported for now at a glance
  • less bugs
    • typo will be reported by the compiler and language servers

cons

  • destroys backward compatibility
    • changing Message.type's type from String to some enum will force us change lots of existing code

I think the disadvantage can be avoided if we create a new field like Message.concreteType with enum type, without changing Message.type's type from String.
However, it may confuse users, since it's hard to understand at a moment that why there're type and concreteType.
For PoC, I made a branch(see here) to see how it goes, and it passes all existing tests, at least.

I would like to hear your opinions. Thank you

@ethicnology
Copy link
Owner

@uchijo

Looks good to me 🙂, at the time I implemented this library I was expecting the nostr to get new messages types.

You can make a PR to develop for this file, and I will rework it a bit.

Of course, we will integrate this as a non-breaking change with depreciation for the String type in a near future

@uchijo
Copy link
Contributor Author

uchijo commented Nov 20, 2023

@ethicnology

Thanks for the feedback!
I'll make pr later today

@ethicnology
Copy link
Owner

I've reworked a bit e8fee85, we improved the code without breaking changes, thank you

Released

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