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

feat: add BatchExternalContact #65

Merged
merged 1 commit into from Feb 1, 2021
Merged

feat: add BatchExternalContact #65

merged 1 commit into from Feb 1, 2021

Conversation

w3xse7en
Copy link
Contributor

@@ -26,6 +26,19 @@ func (c *WorkwxApp) GetExternalContact(externalUserid string) (*ExternalContactI
return &resp.ExternalContactInfo, nil
}

// BatchExternalContact 批量获取客户详情
func (c *WorkwxApp) BatchExternalContact(userID, cursor string, limit int) ([]ExternalContactBatchInfo, string, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

  1. Batch is not a verb, use something like BatchListExternalContacts would be better IMO
  2. a, b Type is not allowed in this repo (like described in this article), duplicating the string part is fine for readability
  3. returning three values is not ergonomic, make a wrapper struct for that is fine

Copy link
Owner

Choose a reason for hiding this comment

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

Like the following

type BatchListExternalContactsResp struct {
    Result []ExternalContactBatchInfo
    NextCursor string
}

P.S. the ExternalContactBatchInfo name is very unfortunate but I cannot come up with a better one, so leave it alone for now...

Copy link
Contributor Author

@w3xse7en w3xse7en Feb 1, 2021

Choose a reason for hiding this comment

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

give a friendly remind, Batch is a verb

Copy link
Owner

Choose a reason for hiding this comment

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

Hah, I forgot it!

Though the "batch" verb is more suited to "batching requests" and such, it doesn't have the "list" meaning implied.

@xen0n
Copy link
Owner

xen0n commented Feb 1, 2021

LGTM overall, thanks for the contribution!

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 1, 2021

Build succeeded:

@bors bors bot merged commit 10e25a0 into xen0n:develop Feb 1, 2021
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.

None yet

2 participants