-
Notifications
You must be signed in to change notification settings - Fork 21
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
Adding mapping between enum keys and values #17
Conversation
Can't actually use types in real code
You should add a test |
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "thrift2flow", | |||
"version": "0.2.12", | |||
"version": "0.2.13", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't bump package version, that will happen after the pull request is landed and a new version is released.
Unit test added |
@keithkml mind taking a look? |
src/main/convert.js
Outdated
} | ||
|
||
return `export type ${this.transformName(def.id.name)}Values = ${this.generateEnumValues( | ||
def | ||
)}; | ||
export type ${this.transformName(def.id.name)} = ${this.generateEnumKeys(def)};`; | ||
export type ${this.transformName(def.id.name)} = ${this.generateEnumKeys(def)}; | ||
export const ${this.transformName(def.id.name)}Map = ${this.generateEnumMap(def)};`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, this kinda defeats the purpose of the type name suffix, which will lead to linting errors. Actually it looks like the previous change which added xxxKeys
broke it too, but I didn't notice. Easy solution is to move the Map
and Keys
suffix into the call to transformName
Example Input:
Output
|
Note this also resolves the eslint error |
src/main/convert.js
Outdated
this.generateImports(), | ||
...this.thrift.asts[this.thrift.filename].definitions.map(this.convertDefinitionToCode) | ||
...this.thrift.asts[this.thrift.filename].definitions.map((value, index, array) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code would look a lot nicer (and this diff would be a lot smaller :) ) if you just stored this as a field, or even just declared a getter function for this value.
Currently for enum generation keys and values are just unions and there is no way to put this data back together. This change will create a map object to allow referencing of one value to the other.