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

bugfix: fix(#85) #86

Merged
merged 2 commits into from
Jun 7, 2021
Merged

bugfix: fix(#85) #86

merged 2 commits into from
Jun 7, 2021

Conversation

cs-charles
Copy link
Contributor

bugfix: fix(#85)

@Allenxuxu
Copy link
Owner

image
感觉是不是在这块后面加上 err 的处理比较合理,之前漏掉了 err 的判断。

@cs-charles
Copy link
Contributor Author

确实,应该在for循环里面加判断会跟加合理

@cs-charles cs-charles closed this Jun 6, 2021
@cs-charles
Copy link
Contributor Author

如果是在for循环开头加上err判断是否可以

for i := 1; i < len(lines); i++ {
            if len(lines[i]) == 0 || err != nil {
	            // Blank line, no more lines to read.
	            break
            }
        ....
}

@Allenxuxu
Copy link
Owner

如果是在for循环开头加上err判断是否可以

for i := 1; i < len(lines); i++ {
            if len(lines[i]) == 0 || err != nil {
	            // Blank line, no more lines to read.
	            break
            }
        ....
}

不光是这里吧,这个函数上面也有 err 漏判的。
这里 for 循环里的 err ,感觉可以 err = xx 然后break,在循环后面统一 if err != nil 处理。

@cs-charles
Copy link
Contributor Author

辛苦大佬有空修复一下这个bug

@Allenxuxu
Copy link
Owner

辛苦大佬有空修复一下这个bug

你不是都写了吗,直接修改下,推上来吧

@cs-charles cs-charles reopened this Jun 7, 2021
@cs-charles
Copy link
Contributor Author

辛苦大佬有空修复一下这个bug

你不是都写了吗,直接修改下,推上来吧

辛苦review一下

@Allenxuxu Allenxuxu merged commit 4f22261 into Allenxuxu:plugin Jun 7, 2021
@Allenxuxu
Copy link
Owner

咦,怎么是向 plugin 分支发起的pr

@cs-charles
Copy link
Contributor Author

咦,怎么是向 plugin 分支发起的pr

我不太确定你们是否是plugin分支开发,然后提交到master,需要更改么。另外想请教你一个问题,我们公司内部的云上的LB会把http请求头的Connection: Upgrade改成Connection: upgrade导致请求升级失败。框架能否修改支持不区分大小写来校验呢

@Allenxuxu
Copy link
Owner

暂时不用改吧,之后我来合到master吧。

大小写的问题,之前有mr修过了,不过还没发tag 似乎。
#82

@Allenxuxu
Copy link
Owner

#38
可以在这里面补充下公司信息吗😄

@cs-charles
Copy link
Contributor Author

#38
可以在这里面补充下公司信息吗😄

补充了,我们近期准备做压测😂,上述两个问题辛苦尽快打一个tag发布。

@cs-charles
Copy link
Contributor Author

#38
可以在这里面补充下公司信息吗😄

抱歉,我刚删除了公司信息,我得先内部问一下此举是否合规😅。如果合规后续我会补上。

@Allenxuxu
Copy link
Owner

#38
可以在这里面补充下公司信息吗😄

补充了,我们近期准备做压测😂,上述两个问题辛苦尽快打一个tag发布。

好,今晚我就发一个

@Allenxuxu
Copy link
Owner

尴尬了,plugin 这个分支暂时还不能合并(这个分支的功能还不太适合合并),你能重新提个 pr 到 master 吗 @cs-charles
合并完我给你打个 tag。

@cs-charles
Copy link
Contributor Author

尴尬了,plugin 这个分支暂时还不能合并(这个分支的功能还不太适合合并),你能重新提个 pr 到 master 吗 @cs-charles
合并完我给你打个 tag。

可以的

@Allenxuxu
Copy link
Owner

@cs-charles v0.2.3 已发布

@Allenxuxu
Copy link
Owner

#38
可以在这里面补充下公司信息吗😄

抱歉,我刚删除了公司信息,我得先内部问一下此举是否合规😅。如果合规后续我会补上。

怎么样,应该合规的吧,可以补上吗😂

@cs-charles
Copy link
Contributor Author

#38
可以在这里面补充下公司信息吗😄

抱歉,我刚删除了公司信息,我得先内部问一下此举是否合规😅。如果合规后续我会补上。

怎么样,应该合规的吧,可以补上吗😂

已加上🤝🤝

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