vadymmarkov / Malibu

:surfer: Malibu is a networking library built on promises
https://vadymmarkov.github.io
Other
414 stars 39 forks source link

Tiny improvement to the request builder #83

Closed OscarApeland closed 6 years ago

OscarApeland commented 6 years ago

If the URL you are requesting is already a valid URL, Malibu shouldn't convert it to String and attempt to create a new URL object.

vadymmarkov commented 6 years ago

@OscarApeland Great 👍 I just have one concern, what if you have a GET request with a valid URL and with parameters dictionary? Then parameters will not be added as query params because buildUrl function is responsible for doing that.

OscarApeland commented 6 years ago

@vadymmarkov I didn't think about that!😅 I can modify it to only use the original URL if the Request.parameters is empty? Just patched this up to fit into our use case where the API returns a full pagination URL and I only need to build an URL the first time around. I can keep it in our own fork if you think it would do more harm than good.

    var request: Request {
        switch self {
        case .threads(let fetcher):
            if let nextUrl = fetcher.nextUrl {
                return Request.get(nextUrl)
            } else {
                return Request.get("threads", parameters: fetcher.parameters)
            }
        }
    }
vadymmarkov commented 6 years ago

@OscarApeland What about:

let url = try {
  if let resourceUrl = resource as? URL {
    return resourceUrl
  } else {
    return concatURL(baseUrl: baseUrl?.urlString)
  }
}

let requestUrl = try buildUrl(from: url)

buildUrl will append all the params from parameters dictionary if there are any.

OscarApeland commented 6 years ago

@vadymmarkov Sounds good, worked for my use case. I altered my PR to what you suggested, just with explicit type as Swift couldn't infer the type. All good?

vadymmarkov commented 6 years ago

Yes, thanks for your contribution @OscarApeland 🚀