Conversation
.gitignore
Outdated
| # JetBrains GoLand | ||
| .idea | ||
| *.iml | ||
| *.ipr No newline at end of file |
There was a problem hiding this comment.
Please don't commit editor-specific ignore rules.
internal/elements.go
Outdated
| s, err := strconv.Unquote(string(b)) | ||
| if err != nil { | ||
| return fmt.Errorf("webdav: failed to unquote ETag: %v", err) | ||
| *etag = ETag(b) |
There was a problem hiding this comment.
Hm, this completely ignores all errors. Not sure this is a great idea.
There was a problem hiding this comment.
Agreed. But how we can logged error with warn status?
|
TBH, maybe we shouldn't do any of the quoting/unquoting stuff. It would be a bit awkward to deal with quoted strings, but at least we wouldn't have this kind of issues and we would handle weak entity tags properly. |
|
Agree. But what will happen with projects, that already use this library? |
|
We've already made plenty of breaking changes since the last release. |
|
ok, changed patch as you said |
emersion
left a comment
emersion
left a comment
There was a problem hiding this comment.
String and MarshalText should be dropped as well, otherwise ETags will get double-quoted.
| *.out | ||
|
|
||
| # Project-local glide cache, RE: https://github.com/Masterminds/glide/issues/736 | ||
| .glide/ |
internal/elements.go
Outdated
| @@ -340,11 +340,7 @@ type GetETag struct { | |||
| type ETag string | |||
|
|
|||
| func (etag *ETag) UnmarshalText(b []byte) error { | |||
There was a problem hiding this comment.
This method is now a no-op and can be dropped.
Done |
- Change separated type ETag to String
|
Hm, this new proposal is error-prone. Library users need to "guess" that they need to double-quote the ETag values they're using. It's very easy to break the RFC with this API. |
|
That's why I worried about remove String method. |
|
|
|
Nope. String() method, that I was removed, is adding quotes to ETag value. |
|
With your changes, the ETag contains quotes already, no need to add them again. |
|
Where my changes add quotes in code ? |
|
Removed wrapper, with call String() method on it. I think it will broke server in setting ETag to headers |
|
Could you please explain, why need write type conversion here?
|
ghost
mentioned this pull request
2 participants
Catched issue same like #50.
Understand that it's needs when server not working strictly by RFC. But some developers will try to use library for "wrong implemented" servers and get the same error.
Think it's possible to make requirement not so strictly in that case.