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

Notification and user methods version 2 #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vlamug
Copy link

@vlamug vlamug commented Aug 9, 2017

No description provided.

type UserContact struct {
ID string `json:"id"`
To string `json:"to"`
ContactMethod string `json:"contactMethod"`
Copy link
Contributor

Choose a reason for hiding this comment

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

given this value is from a set of fixed values - are there plans to make this a Constant/Enum?

Copy link
Author

@vlamug vlamug Aug 9, 2017

Choose a reason for hiding this comment

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

I defined these fixed methods in the file:
notificationv2/constants.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. I was more meaning to use the Constants as the Type of the ContactMethod field in the Struct, in place of String?

Copy link
Author

Choose a reason for hiding this comment

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

It looks great. Ok, I will change it soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks @vlamug :)

@vlamug vlamug force-pushed the notification_and_user_v2 branch from ec49053 to 6e3a273 Compare August 14, 2017 14:21
@vlamug
Copy link
Author

vlamug commented Aug 14, 2017

@tombuildsstuff hello, I added necessary constants. Please, check it.

Thanks.

@mustafanacar
Copy link
Contributor

Hi @vlamug , thanks for your contribution. I'll be looking into your pull request, sorry for the delay. I spotted some minor errors/missing elements and will inform you when I have time to do an in depth review.

@vlamug
Copy link
Author

vlamug commented Sep 13, 2017

@mustafanacar hello, how about making code-review? thanks.

@omerozkan omerozkan self-assigned this Oct 10, 2017
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

Successfully merging this pull request may close these issues.

4 participants