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

New LIFF API PR #89

Closed
justdomepaul opened this issue Jul 10, 2018 · 3 comments
Closed

New LIFF API PR #89

justdomepaul opened this issue Jul 10, 2018 · 3 comments

Comments

@justdomepaul
Copy link
Contributor

justdomepaul commented Jul 10, 2018

my new PR: #83

I PR a latest version, please code review and merge to master.

Please close my Before issue: #84

Thanks.

@sugyan
Copy link
Contributor

sugyan commented Jul 10, 2018

Thanks for updating your pull-request.
It seems almost good, but I have some comments for improvement.

  1. liff.go has some problems with comments and field names.
linebot/liff.go:26:6: exported type LIFFAPP should have comment or be unexported
linebot/liff.go:31:6: exported type ViewRequest should have comment or be unexported
linebot/liff.go:35:6: exported type View should have comment or be unexported
linebot/liff.go:37:2: struct field Url should be URL
linebot/liff.go:40:1: comment on exported method Client.GetLIFF should be of the form "GetLIFF ..."
linebot/liff.go:71:1: comment on exported method Client.AddLIFF should be of the form "AddLIFF ..."
linebot/liff.go:118:1: comment on exported method Client.UpdateLIFFCall should be of the form "UpdateLIFFCall ..."
linebot/liff.go:119:38: method parameter liffId should be liffID
linebot/liff.go:146:3: struct field Url should be URL
linebot/liff.go:171:1: comment on exported method Client.DeleteLIFFCall should be of the form "DeleteLIFFCall ..."
linebot/liff.go:172:38: method parameter liffId should be liffID

please install and run golint (golang.org/x/lint/golint) and check outputs.

  1. method name has no consistency.
    I think func (client *Client) UpdateLIFFCall and func (client *Client) DeleteLIFFCall should be named UpdateLIFF and DeleteLIFF, which shouldn't include Call.

and...

This pull-request seems to contain many commits unrelated to LIFF.
I'd be happy if you make different pull-request which only include relevant changes as separate branches.

Did you sign the ICLA? https://feedback.line.me/enquete/public/919-h9Yqmr1u
Please submit that if you didn't submit yet.

@justdomepaul
Copy link
Contributor Author

Ok, I will modify these issue, rebase commit and push latest version.

@justdomepaul
Copy link
Contributor Author

justdomepaul commented Jul 11, 2018

#90

@sugyan

I separate a new alone "liff api" branch, golint file, and change func name to your suggestion.

Please code review and merge, thanks.

ps. I have applied ICLA recently.

@sugyan sugyan closed this as completed Jul 11, 2018
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

No branches or pull requests

2 participants