Split OWM API or make it more explicit about valid URLs

[asoltysik]

As far as I know it's not currently possible to have the whole API working at the same time - when the passed url is history.openweathermap.org, only methods history* work. When passed pro.openweathermap.org or api.openweathermap.org, only forecast* and current* methods succeed. The concept of passing custom urls also does not seem useful to me, as there are only 3 choices (correct me if I'm wrong here).

My proposition:
Currently Client is a trait that has one abstract method receive. The rest are concrete methods calling receive. We can split them and hardcode API urls inside traits, then use them traits as mixins. Quick simplified proof of concept:

// simplified, dummy case classes
case class Request()
trait Response
case class History() extends Response
case class Current() extends Response
case class Forecast() extends Response

trait Transport[F[_]] {
	def receive[W <: Response](owmRequest: Request): F[Either[Throwable, W]]
}

trait HistoryClient[F[_]] extends Transport[F] {
  def historyByName(name: String): F[Either[Throwable, History]] = receive[History](Request())
}

trait WeatherClient[F[_]] extends Transport[F] {
  def currentByName(name: String): F[Either[Throwable, Current]] = receive[Current](Request())
  def forecastByName(name: String): F[Either[Throwable, Forecast]] = receive[Forecast](Request())
}

class AsyncClient[F[_]] extends Transport[F] {
  def receive[W <: Response](owmRequest: Request): F[Either[Throwable, W]] = ??? // do stuff
}

class CacheClient[F[_]](asyncClient: AsyncClient[F]) extends Transport[F] {
  def receive[W <: Response](owmRequest: Request): F[Either[Throwable, W]] = ??? // do stuff
}

//////////////////
// factories:
//////////////////

def createAsyncHistoryClient[F[_]] = new AsyncClient[F] with HistoryClient[F]
def createAsyncClient[F[_]] = new AsyncClient[F] with HistoryClient[F] with WeatherClient[F]

[BenFradet]

The concept of passing custom urls also does not seem useful to me

I think this is not to have to release a new version if the urls change but I do agree. One way to solve this would be to have an implicit holding the default urls.

---

I do like the idea overall but we have to consider a couple of things:

• does it make integrating other providers more difficult or easier? (c.f. Integrate darksky as a weather provider #42)
• does having many entrypoints to the api make things more difficult for the end user, we had 2 we'll have 6 (if I'm not mistaken)

cc @chuwy

[asoltysik]

does it make integrating other providers more difficult or easier? (c.f. #42)

IMHO easier because after the split Transport can be easily made provider-agnostic. All of providers' Clients would be then subclasses of Transport.

does having many entrypoints to the api make things more difficult for the end user, we had 2 we'll have 6 (if I'm not mistaken)

Probably more confusing, that's true, the above point still stands though. I've probably overcomplicated this. We don't need "history" client, "current/forecast" client and "both" client, we just need "current/forecast" client and "both" client, because:

• if user didn't buy the historical plan and is either on free or paid plan, he has only access to current/forecast API
• if user bought the historical plan, he obviously has access to the history API, but the same api key can be used to access free current/forecast API

[BenFradet]

if user bought the historical plan, he obviously has access to the history API, but the same api key can be used to access free current/forecast API

that's something we'd need to differentiate on but I agree on the other points 👍

[chuwy]

Few points from me:

• I agree with @BenFradet that we need to have an ability to use custom URL in case provider changes something. It can be not first and most apparent thing in public API, but it should be available somewhere.
• I don't have a strong opinion on splitting client traits, but in case we're going to do this, I'd like to not expose these traits as first-class public APIs, but instead have just those factories @asoltysik mentioned.
• In the end when we'll start to add another providers, I don't believe we'll be able to preserve enough abstraction for requests/responses: e.g. some providers will support only geo coordinates, some only city names, some only city-countries pairs etc.
• Having more specific subclasses in return type (Forecast as opposed to Response) usually not that good idea as it seems at first glance: it breaks the concept of ADT with inheritance and it is easy to mess up with variance.

[asoltysik]

Part of this issue will be covered by #71