DEVEXP-381: Flexible enums for regions#54
Conversation
…Client-parameters
|
|
||
| private buildHostname(region: ConversationRegion) { | ||
| private buildHostname(region: ConversationRegionFlexible) { | ||
| const formattedRegion = region === '' ? region : `${region}.`; |
There was a problem hiding this comment.
I think it come from myself, but thank you to small function usage here: my first reading of this line was cause of reflexion to understand logic and thinking there was a issue:
- testing against an empty value to use the testing value (the empty one)
- if not empty: use it too....
🤔
Then I realized the final dot for second case; then use to format a string
Just a personal thought I think I would have written logic as
const formattedRegion = region !== '' ? `${region}.` '';
Which could highlight in a better way the logic and use case.
Again just a personal thought and not a blocker
There was a problem hiding this comment.
You are right, it would be more readable.
There was a problem hiding this comment.
Fixed for all 4 APIs that use regions
| console.error(`No region exist for the value '${value}'`); | ||
| return undefined; | ||
| } | ||
| export type SmsRegionFlexible = SmsRegion | string; |
There was a problem hiding this comment.
By inverting the point of view; could the type being more user friendly ?
I mean:
export enum SupportedSmsRegion {
UNITED_STATES = 'us',
EUROPE = 'eu',
BRAZIL = 'br',
CANADA = 'ca',
AUSTRALIA = 'au'
}
export type SmsRegion = SupportedSmsRegion | string;So when function prototype or variable usage will reference the type, it will be named SmsRegion.
Seems to be more "user friendly" than a SmsRegionFlexible reference and usage.
What do you think?
There was a problem hiding this comment.
This would be more understandable indeed. The issue being that with this implementation, I can't do SmsRegion.EUROPE as SmsRegion refers to a Type, but would be used as a value
A solution would be to trick the user and define on top of the enum a new object SmsRegion such as
export const SmsRegion = {
...SupportedSmsRegion,
};
WDYT?
There was a problem hiding this comment.
As usual, we should have end user and SDK usage in mind first.
so if a SmsRegion type is identified as the best way to reach this objective, if by implementing it as you described in your comment above: it is very fine
Then, using same pattern for Conversation, if SDK usage enable to store a variable/use a parameter type named ConversationRegion and reference ConversationRegion.UNITED_STATES or use us directly : it should provide a better DX (IMO)
There was a problem hiding this comment.
Ok, we can do that. Keep in mind the trick I mentioned above: ConversationRegion.UNITED_STATES will not reference the enum, but an object of which we read the value associated to the key UNITED_STATES 😉
2 participants
No description provided.