-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
net/url: improve url parsing error messages by quoting #29384
Conversation
Current implementation doesn't always make it obvious what the exact problem with the URL is, so this makes it clearer by consistently quoting the invalid URL, as is the norm in other parsing implementations, eg.: strconv.Atoi(" 123") returns an error: parsing " 123": invalid syntax Updates golang#29261
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
CLA process initiated with my employer. |
I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
This PR (HEAD: 1b84002) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
Message from Stefan Baebler: Patch Set 1: Hey! This is the simplest fix for #29261 It does not attempt expand the parsing algorithm to give more granular error messages, but it improves the error message by enclosing the problematic URL in quotes, consistently with other parsing functions. If the URL is empty or it starts or ends with a whitespace the improved error message should give ample hints about the root problem. Since you are familiar with both:
I am asking you to review this patch. I do agree that the URL parsing should eventually be improved, but it does not seem this will happen for Go 1.13 release as planned. This simpler fix should be safer to merge without introducing bugs or breaking changes and should stay in the code even after more thorough URL parsing is implemented. Thanks, Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
This PR (HEAD: 342659e) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: 96c7a0a) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: a907fae) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
Message from Martin Möhrmann: Patch Set 4: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
This PR (HEAD: ef428f9) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
Message from Stefan Baebler: Patch Set 5: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
This PR (HEAD: bc499f3) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
This PR (HEAD: 3f3607c) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
Message from Daniel Martí: Patch Set 11: (5 comments) Some nits; we can test once those are fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
This PR (HEAD: 648b9d9) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/185117 to see it. Tip: You can toggle comments from me using the |
Message from Štefan Baebler: Patch Set 12: (5 comments) Thanks for the suggestions, all fixed! Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
Message from Daniel Martí: Patch Set 12: Run-TryBot+1 Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
Message from Gobot Gobot: Patch Set 12: TryBots beginning. Status page: https://farmer.golang.org/try?commit=a820b954 Please don’t reply on this GitHub thread. Visit golang.org/cl/185117. |
Current implementation doesn't always make it obvious what the exact problem with the URL is, so this makes it clearer by consistently quoting the invalid URL, as is the norm in other parsing implementations, eg.: strconv.Atoi(" 123") returns an error: parsing " 123": invalid syntax Updates #29261 Change-Id: Icc6bff8b4a4584677c0f769992823e6e1e0d397d GitHub-Last-Rev: 648b9d9 GitHub-Pull-Request: #29384 Reviewed-on: https://go-review.googlesource.com/c/go/+/185117 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org>
This PR is being closed because golang.org/cl/185117 has been merged. |
Current implementation doesn't always make it obvious what the exact problem with the URL is, so this makes it clearer by consistently quoting the invalid URL, as is the norm in other parsing implementations, eg.: strconv.Atoi(" 123") returns an error: parsing " 123": invalid syntax Updates golang#29261 Change-Id: Icc6bff8b4a4584677c0f769992823e6e1e0d397d GitHub-Last-Rev: 648b9d9 GitHub-Pull-Request: golang#29384 Reviewed-on: https://go-review.googlesource.com/c/go/+/185117 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org>
Current implementation doesn't always make it obvious what the exact problem with the URL is, so this makes it clearer by consistently quoting the invalid URL, as is the norm in other parsing implementations, eg.: strconv.Atoi(" 123") returns an error: parsing " 123": invalid syntax Updates golang#29261 Change-Id: Icc6bff8b4a4584677c0f769992823e6e1e0d397d GitHub-Last-Rev: 648b9d9 GitHub-Pull-Request: golang#29384 Reviewed-on: https://go-review.googlesource.com/c/go/+/185117 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Run-TryBot: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: Gobot Gobot <gobot@golang.org>
error message returned from net/http now quoted since github.com/golang/go/pull/29384 fix the unit test as it become fail when asserting the error msg
error message returned from net/http now quoted since github.com/golang/go/pull/29384 fix the unit test as it become fail when asserting the error msg
This actually caused quite a few errors in our unit tests due to quoting... |
Us too. See https://golang.org/cl/195978. We decided to bite the bullet and fix the tests. |
Yeah, in the end it wasn't that hard to patch the tests, but it was surprising :) |
…et/url.Error is now quoted Updates #37614 Updates #36878 Updates #29384 Fixes #37630 Change-Id: I63dad8b554353197ae0f29fa2a84f17bffa58557 GitHub-Last-Rev: 5297df3 GitHub-Pull-Request: #37661 Reviewed-on: https://go-review.googlesource.com/c/go/+/222037 Reviewed-by: Ian Lance Taylor <iant@golang.org> (cherry picked from commit 2b0f481) Reviewed-on: https://go-review.googlesource.com/c/go/+/222317 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Current implementation doesn't always make it obvious what the exact
problem with the URL is, so this makes it clearer by consistently quoting
the invalid URL, as is the norm in other parsing implementations, eg.:
strconv.Atoi(" 123") returns an error: parsing " 123": invalid syntax
Updates #29261