这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@alexwennerberg
Copy link
Contributor

Textproto should throw an error and fail to parse a header if it
contains an invalid rather than preserving it as-is.

References: #68

I have very little Go experience, so let me know if there's any issues with this fix or if anything is non-idiomatic. I'm happy to clean things up :)

Textproto should throw an error and fail to parse a header if it
contains an invalid rather than preserving it as-is.

References: #68
@codecov-io
Copy link

codecov-io commented Jan 19, 2020

Codecov Report

Merging #69 into master will increase coverage by 0.53%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
+ Coverage      62%   62.54%   +0.53%     
==========================================
  Files          15       15              
  Lines         858      865       +7     
==========================================
+ Hits          532      541       +9     
+ Misses        280      279       -1     
+ Partials       46       45       -1
Impacted Files Coverage Δ
textproto/header.go 80.57% <100%> (+1.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47cbba6...a1023e0. Read the comment docs.

Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Thanks for you pull request! You're right, we should validate the header key against RFC 5322 here.

This code originally comes from Go's stdlib, which I guess implemented it for HTTP. This is why there are references to HTTP's RFC, although we should reference RFC 5322 instead. tl;dr historical raisins as some say.

The gist of the PR LGTM, here are a few comments.

@alexwennerberg
Copy link
Contributor Author

alexwennerberg commented Jan 20, 2020

I’ll address these changes as soon as I can — probably tomorrow evening. Thanks!

Minor mostly cosmetic changes based on PR comments

Restructuring test case -- removing string check
@alexwennerberg
Copy link
Contributor Author

OK, I think I fixed everything you mentioned! :)

Copy link
Owner

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@emersion emersion merged commit 8ade7dd into emersion:master Jan 22, 2020
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.

3 participants