Skip to content

Add type for choicefield #7161

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

Merged
merged 10 commits into from
Jan 29, 2020
Merged

Add type for choicefield #7161

merged 10 commits into from
Jan 29, 2020

Conversation

dhaval-mehta
Copy link
Contributor

@dhaval-mehta dhaval-mehta commented Jan 25, 2020

@carltongibson @kevin-brown As we have discussed in #7080, In majority cases, the type of enum values for the choicefield will be one of the following.

  1. string
  2. int
  3. float

In this merge request, I have added code to generate the type for majority cases.

Copy link
Member

@kevin-brown kevin-brown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small changes but otherwise I think this is a reasonable approach to getting this done.

@kevin-brown kevin-brown dismissed their stale review January 27, 2020 23:34

Changes have been made.

@dhaval-mehta
Copy link
Contributor Author

@carltongibson Waiting for your valuable review.

@carltongibson
Copy link
Collaborator

Hi @dhaval-mehta. Please don´t do that. I know. (And thanks for your effort!)

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, OK. This looks good @dhaval-mehta. Welcome aboard! ⛵️

Thanks for the reviews @kevin-brown 🥇

@carltongibson carltongibson merged commit bc4d525 into encode:master Jan 29, 2020
@dhaval-mehta dhaval-mehta deleted the add-type-for-choicefield branch January 29, 2020 22:08
@dhaval-mehta
Copy link
Contributor Author

@carltongibson @kevin-brown This is my first open-source contribution. Thanks for your guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants