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

Add AccountLink event #91

Merged
merged 5 commits into from Jul 12, 2018
Merged

Add AccountLink event #91

merged 5 commits into from Jul 12, 2018

Conversation

suzuki-shunsuke
Copy link
Contributor

No description provided.

@sugyan sugyan self-requested a review July 12, 2018 06:18
linebot/event.go Outdated
Message Message
Postback *Postback
Beacon *Beacon
AccountLink *AccountLink `json:"link"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this json tag needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry. This tag is not needed.

linebot/event.go Outdated
@@ -82,25 +83,33 @@ type Beacon struct {
DeviceMessage []byte
}

// AccountLink type
type AccountLink struct {
Result string
Copy link
Contributor

Choose a reason for hiding this comment

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

Result seems to be ok or failed only.
Can you modify its type from string to other type and use constant values?

For example:

type AccountLinkResult string

const (
	AccountLinkResultOK	AccountLinkResult = "ok"
	AccountLinkResultFailed	AccountLinkResult = "failed"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I see.
I will fix them and add tests of Event.MarshalJSON and Event.UnmarshalJSON.
And I have to fix Event.MarshalJSON also.

@suzuki-shunsuke
Copy link
Contributor Author

I have fixed.

@@ -236,6 +236,19 @@ var webhookTestRequestBody = `{
"type":"enter",
"dm":"1234567890abcdef"
}
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please replace tabs to spaces...? (Here is just inside the JSON literal..)

UserID: "U012345678901234567890123456789ab",
},
AccountLink: &AccountLink{
Result: "ok",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Result: AccountLinkResultOK is more better.

@suzuki-shunsuke
Copy link
Contributor Author

@sugyan Thank you for your review.
I have fixed again.

@sugyan
Copy link
Contributor

sugyan commented Jul 12, 2018

LGTM. Thanks! 🙆‍♂️

@sugyan sugyan merged commit e0923b0 into line:master Jul 12, 2018
@suzuki-shunsuke suzuki-shunsuke deleted the add-accountlink-event branch January 1, 2021 06:19
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.

None yet

2 participants