Add header array parameter support #1
Conversation
How so? 😸 |
inkel
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I've left some comments, I'd like for us to discuss them if you are not convinced by them.
main.go
Outdated
| "strings" | ||
| ) | ||
|
|
||
| type stringArray []string |
There was a problem hiding this comment.
Why not using a map[string][]string instead? Also the type name, while correct, is too generic, maybe headersSomething?
There was a problem hiding this comment.
Changed to MIMEHeader from textproto which is map[string][]string. Also renamed. I put that name before because I thought of this as a generic string array argument parser. But this makes more sense.
main.go
Outdated
|
|
||
| for _, h := range headerFlags { | ||
| ha := strings.Split(h, ": ") | ||
| if i := w.Header().Get(ha[0]); i != "" { |
There was a problem hiding this comment.
I think that whether using a slice or a map, we could always use w.Header().Add. WDYT?
main.go
Outdated
| http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { | ||
| w.WriteHeader(*code) | ||
|
|
||
| for _, h := range headerFlags { |
There was a problem hiding this comment.
A nice benefit of defining the type as a map is that here you have for key, values := range …, which are more expressive than h and ha 😉
|
On an unrelated note, |
|
I used net/textproto for the type.
So I guess that is not that you can't but it doesn't work out of the box (content type is not working on the original version). |
main.go
Outdated
| } | ||
|
|
||
| func (i *headersMap) Set(value string) error { | ||
| v := strings.Split(value, ": ") |
There was a problem hiding this comment.
Last change request: use strings.SplitN instead, as strings.Split would fail to set the proper value in Foo: Bar:Baz (it would return a slice of len 3). I forgot about it in the previous review 😊
main.go
Outdated
| } | ||
|
|
||
| func main() { | ||
| var headerFlags headersMap = make(map[string][]string) |
There was a problem hiding this comment.
I've just realized, why not headers instead of headerFlags?
|
Updated |
Also