Vaidate URLs #2896
Vaidate URLs #2896
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2896 +/- ##
==========================================
- Coverage 98.10% 91.77% -6.34%
==========================================
Files 142 142
Lines 12340 14695 +2355
==========================================
+ Hits 12106 13486 +1380
- Misses 159 809 +650
- Partials 75 400 +325
|
There was a problem hiding this comment.
Thank you, @WillAbides .
I'm going to stop review now because I'm curious if you are able to see all the Codecov warnings that I'm able to see?
If not, I can highlight them for you, as there are a lot, as also evidenced by the significant drop in code coverage stats. It also found some errors, so I'll let you clean those up before I continue reviewing.
Thanks!
| u, err := newURLString("notifications") | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
I don't see any benefit in this case. I think we can safely revert this change.
| } else { | ||
| u = "user/starred" | ||
| u, err = newURLString("user/starred") |
There was a problem hiding this comment.
This line can stay as it was before. Any time newURLString has only a single constant arg, that change can be reverted.
|
@gmlewis Thanks for having a look. I knew this would likely have a negative impact on code coverage, but decided to wait on the PR for codecov to show me the diff before worrying about it. As an aside now that I've looked at the coverage, 98.1% coverage on master is impressive. For the urls that are just static strings, I decided to replace those as well so that we are using a consistent pattern across all methods. I don't feel strongly about that and am happy to revert those. I'm having doubts about the general usefulness of this PR. It seems like if we catch this error in either |
Honestly, I am totally fine if you want to call this an interesting experiment and determine that the results are not worth it. I remember filing #1203 after discovering that an error wasn't triggered when I messed something up which sent alarms through my mind, and was hoping that someone else would give it a good investigation and come to a conclusion, which you graciously have... and thank you! If you wish to point to this PR in that issue and close them both as "unnecessary", I'm totally fine with that and once again really appreciate your detailed investigation! |
|
Closing this as unnecessary. I also commented on #1203, but I can't actually close it myself. |
Resolves #1203
This addresses the issue raised in #1203 by adding a
newURLStringfunction that builds a url the same way they were previously built withfmt.Sprintfbut also errors if the result is not valid.I went this route instead of modifying
addOptionsbecause it seemed better to validate all urls instead of only the ones that useaddOptions.This turned into a pretty big diff, but that is the nature of updating boilerplate that is used in hundreds of methods.