Add Absolute url matcher#254
Conversation
Liquidsoul
left a comment
Liquidsoul
left a comment
There was a problem hiding this comment.
Hi @victorg1991
Thank you for your contribution following the guidelines and templates 🎉
I am wondering if we should add this new function as-is or if it should be renamed.
Let's summon @AliSoftware to see what he thinks about that 😁
| # OHHTTPStubs — CHANGELOG | ||
|
|
||
| ## Master | ||
| Added absoulte url matcher. |
| ## Master | ||
| Added absoulte url matcher. | ||
| [@victorg1991](https://github.com/victorg1991 | ||
| [#252](https://github.com/AliSoftware/OHHTTPStubs/pull/252) |
There was a problem hiding this comment.
The number of this PR is 254 😉
| * | ||
| * - Returns: a matcher (OHHTTPStubsTestBlock) that succeeds only if the request | ||
| * has the given absolute url | ||
| */ |
There was a problem hiding this comment.
I am wondering if this should not be called isAbsoluteString to match the URL member we call underneath, because there is a URL.absoluteURL
🤔
There was a problem hiding this comment.
Totally. We tried to name most if not all our matchers from the name of the method we call underneath to make the relationship clear, use common vocabulary and avoid confusion so let's try to continue that way 😉
There was a problem hiding this comment.
I think that isAbsoluteUrl has more meaning than the other option, but its up to you.
Do I have to change to isAbsoluteString? I could also do something like URL.absoluteURL.absoluteString ¯_(ツ)_/¯ haha
What do you think?
There was a problem hiding this comment.
The problem with isAbsoluteUrl is also that it doesn't follow the Swift naming convention, letting us believe that the expected parameter is an URL and not a String.
Otoh just "isAbsoluteString" without context (as it's a free function, not a method on a tour) is also ambiguous… maybe isAbsoluteURLString? But might be too long. Tbh I honestly don't know what's best suited (especially at 0:30am 😅)
There was a problem hiding this comment.
I prefer isAbsoluteURLString over isAbsoluteString, it doesn't sound too bad :)
There was a problem hiding this comment.
So I rename to isAbsoluteURLString? @AliSoftware
There was a problem hiding this comment.
yeah seems like a good compromise 👍
There was a problem hiding this comment.
Don't forget the * to create a bullet point
There was a problem hiding this comment.
Maybe use a more contrived example like with x=1&y=2#anchor query + anchor?
| * | ||
| * - Returns: a matcher (OHHTTPStubsTestBlock) that succeeds only if the request | ||
| * has the given absolute url | ||
| */ |
There was a problem hiding this comment.
Totally. We tried to name most if not all our matchers from the name of the method we call underneath to make the relationship clear, use common vocabulary and avoid confusion so let's try to continue that way 😉
There was a problem hiding this comment.
Maybe add one or two more complex examples, I'm especially thinking about URLs with query parameters and anchors etc
|
Done! |
Liquidsoul
left a comment
Liquidsoul
left a comment
There was a problem hiding this comment.
Just a typo in the CHANGELOG I missed earlier and I think we should be good to go 😃
|
|
||
| ## Master | ||
| * Added absolute url matcher. | ||
| [@victorg1991](https://github.com/victorg1991 |
There was a problem hiding this comment.
Typo here, missing closing parenthesis ✏️
There was a problem hiding this comment.
Dish! I'll fix tha in a minute : D
| #if swift(>=3.0) | ||
| let req = URLRequest(url: URL(string: url)!) | ||
| #else | ||
| let req = NSURLRequest(URL: NSURL(string: url)!) |
There was a problem hiding this comment.
Unrelated to this PR directly, but I see we have that pattern on multiple tests: maybe we should add a func request(from string: String) -> URLRequest helper function for our tests, responsible for handling Swift 2/3 compatibility, and use that in our test cases to avoid repeating those #if Swift(>=3.0) everywhere? (Might be a separate PR though)
There was a problem hiding this comment.
Yes, I agree. But I think this should be in another PR.
3 participants
Checklist
Description
This PR adds a matcher for the url matcher
Motivation and Context
Sometimes it's useful to match urls by the absolute url
I've created a test to changes the changes and run all the test 100% success