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

重构SDK代码 #3

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

重构SDK代码 #3

wants to merge 14 commits into from

Conversation

gotoxu
Copy link

@gotoxu gotoxu commented Dec 2, 2017

新的SDK代码在yunpian文件夹下,请评审。
新接口的调用方式,以发送单个SMS短信为例:

cfg := DefaultConfig().WithAPIKey("" /*you api key*/)
sms := NewClient(cfg).SMS()

input := &SingleSendRequest{
	Mobile: "13800138000",
	Text:   "您的验证码是1234。如非本人操作,请忽略本短信",
}

resp, err := sms.SingleSend(input)
if err != nil {
	// handle error
	return
}

if !resp.IsSuccess() {
	// 发送失败
	return
}

// 正常业务流程

目前只实现了V2接口,如果需要实现V1接口,请自行实现。
由于我没有测试API KEY,因此测试用例不是很齐全,如果需要请自行补齐。

@gotoxu gotoxu mentioned this pull request Dec 2, 2017
@dzh
Copy link
Contributor

dzh commented Dec 4, 2017

感谢贡献的pull!

有3个问题:
1)我们希望接口能返回统一的格式,至少类似包含如下内容:{code:接口处理码,data:返回数据,msg:code的描述信息,detail:异常时的详情} . 现在的接口返回格式其实是历史问题,后面的接口基本照上面的方式返回
2) 这个pull里只包含了接口正常返回时的处理,有些接口在code!=0时返回的格式和code=0不同(这也是一个历史问题),所以code!=0时的代码就解析不对了
3)不支持v1接口,并且希望在sdk层面能近可能统一v1和v2的处理方式。

基于上面的分析,暂时无法合并到分支。如果上面这些问题能解决的话,我们考虑合并到分支,或者把你的sdk链接放在我们的官方第三方sdk库列表里。

@gotoxu
Copy link
Author

gotoxu commented Dec 4, 2017

  1. 这是一个不好的设计,因为你的接口实际返回值的结构并不相同,比如你的批量操作接口返回的JSON结构的顶层就并没有code,而是将code放入了data数组中,并且将data数组中的code在客户端强行统一到一个顶层的code中在我看来也并不是一个好的设计。即使在客户端将所有接口的返回值进行统一后,如果服务器的某个单独的接口发生变更,那么修改SDK代码就会影响到其他接口。
  2. 4xx返回值的处理代码在这里:
func (c *Client) handleResponse(resp *http.Response, out interface{}) error {
	if resp.StatusCode >= http.StatusBadRequest {
		var err ErrorResponse
		if e := c.decodeJSONBody(resp, &err); e != nil {
			return e
		}
		return err
	}

	return c.decodeJSONBody(resp, out)
}

其中,ErrorResponse的定义为:

// ErrorResponse is the return format when the response fails
type ErrorResponse struct {
	HTTPStatusCode int    `json:"http_status_code"`
	Code           int    `json:"code"`
	Message        string `json:"msg"`
	Detail         string `json:"detail"`
}

func (resp ErrorResponse) Error() string {
	return fmt.Sprintf("请求失败,StatusCode: %d, Code: %d, Message: %s, Detail: %s", resp.HTTPStatusCode, resp.Code, resp.Message, resp.Detail)
}

在SDK中将4xx返回也是当作错误进行处理。

  1. V1和V2的兼容请你们自行处理,我并没有找到V1接口的文档,所以无法开发。

@dzh
Copy link
Contributor

dzh commented Dec 4, 2017

回答一下你的问题:
1.我们的接口以后都会统一下到上面说的格式中,且sdk把接口没做好的事屏蔽到统一处理是合理的方式。
2.“如果服务器的某个单独的接口发生变更,那么修改SDK代码就会影响到其他接口。” 现有sdk代码支持通过ResultHandler定制解析服务端返回且兼容目前所有的接口返回格式,所以不会影响到其他接口
3.目前的rest接口因历史原因存在一些问题,比如同一个接口在可能返回3种格式的json格式,目前的pull请求无法在一种统一的编程下处理这些问题,且不兼容v1和v2的话,对其他使用者的工作量还是较大的,显然不太合适。v1的接口定义在官网https://www.yunpian.com/api/user.html

我们对SDK的基本要求如下:
1)合理的代码组织,保证v2接口的同时,尽量能兼容v1
2)统一的返回格式(code目前是必须的)
3)较完整的单元测试 (我们目前没提供对外的测试服务,资源和人力都有限制,请谅解)

感谢你的改进,可以考虑把你的sdk作为我们的第三方sdk库,供用户选择

@gotoxu
Copy link
Author

gotoxu commented Dec 4, 2017

@dzh

关于第二个问题,我们还是认为目前来说统一的返回值格式不是一个好的设计,当然如果你的意思只是要求所有的返回值中必须有一个code字段,那么我可以处理,但是如果要求改成像你们sdk里那样统一返回Result结构体,那么我们不会这样实现。因为实际上你的接口返回值并没有统一,即使将来统一了,再修改sdk也可以。但是现在强行在客户端统一会对用户使用造成不变。比如,如果目前要实现统一的返回值,那么你Result结构体中的Data字段就必须定义为 interface{} , 这对于用户要解析Data中数据的操作就会造成困难,实际上你的SDK代码中也大量使用了反射,我们认为这并不是一个好的实现方式。

如果你执意要求按照统一的Result返回值格式定义,那么请关闭该PR,我们将自行使用我们自己开发的SDK,如果你可以接受我们的建议,那么我再继续实现V1接口并完善单元测试,谢谢

@dzh
Copy link
Contributor

dzh commented Dec 4, 2017

@Jamesxql
你说的接口返回指定struct是可以的,如果能v1和v2都兼容实现的话,用户使用比较方便,我们到时可以作为第三方sdk推荐给用户。

现有sdk的设计,是目前能较好兼容现在和将来的API结构的设计,暂时看不需要改变。
而Result的data部分可以通过继承后扩展实现解析成指定具体的struct,只需在现有的代码上定制就可以,这个在以前开发时就已经考虑了,目前没有做,只是没有时间针对所有接口做一个struct而已。

@dzh
Copy link
Contributor

dzh commented Dec 4, 2017

@Jamesxql
若功能基本完整,以后功能单元测试也完整,发一个你的仓库地址链接,我们可以先放到README.md

@gotoxu
Copy link
Author

gotoxu commented Dec 4, 2017

@dzh V1 接口已经实现,我们仓库的链接地址: https://github.com/FeiniuBus/yunpian-go-sdk/tree/master/yunpian

@dzh
Copy link
Contributor

dzh commented Dec 6, 2017

@Jamesxql 链接已加到README的“开源SDK列表”

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.

2 participants