Base url required on builder. Should be in constructor of builder.

[clockworkant]

Looking at the Retrofit Builder it has a mandatory field of BaseUrl. With this in mind shouldn't the BaseUrl be part of the constructor or the Builder class?

[JakeWharton]

I have two arguments against this. Neither are particularly strong, but they're based on history of working on Retrofit (and other libs) and the fact that counterarguments were never stronger (or just were never made).

• What is required and what is optional may change in future versions. Now I'll admit that this is less likely than it was in 1.x, but there's a remote possibility we'd require some crazy new dependency in a minor update. Since base URL is the only required dependency, it's highly unlikely that we'd relax the requirement.

  That said, we'll be doing a Retrofit 3.0 when OkHttp goes 3.0 to track its breaking API changes and any change as I mentioned is not only highly unlikely to happen in that period, but also would allow another get-out-of-API-free card when we do this update. Shit, I'm counterarguing myself...

• Requiring the base URL on both a builder constructor or builder factory method prevents using the descriptive name "base URL" anywhere but in the Javadoc. This will be slightly important as every HTTP method annotation (e.g., @GET) and the @Url annotation will reference "base URL" many times in detailing on what base their relative URLs are resolved.

  So on a constructor you get

  new Retrofit.Builder("http://example.com/api/")

  and for a static factory

  Retrofit.builder("http://example.com/api/")

  which definitely isn't terrible.

  But, most dependency resolution systems are going to only download the binary jar when you specify a Retrofit dependency. This means when you call new Retrofit.Builder() and hit CMD+P (or whatever) you'll only see String s. Not very helpful!

  With a static factory there's also always the option of being painfully verbose

  Retrofit.builderWithBaseUrl("http://example.com/api/")

  So the thinking behind the method is mostly that it's self-documenting. It tells you exactly what the supplied argument value should be and also hopefully hammers home the "base URL" so that when you see it in our extensive (and still forthcoming) documentation it clicks.

  new Retrofit.Builder()
    .baseUrl("http://example.com/api/")

Like I said I'm not married to the current choice, but I at least have some reason for preferring it even if they are relatively weak compared to other API decisions in the library. I know @f2prateek is on board with changing the API, and I had given similar arguments when he sent a PR for it.

I'll leave the burden of deciding on whoever wants to chime in here.

[artem-zinnatullin]

Another option — add separate Builder class with only one method public CompleteBuilder baseUrl(String baseUrl) so it won't be possible to forget baseUrl and call build(). This option allows you keep almost all pros of regular Builder pattern.

We use this in StorIO (yeah, yeah) for all buildable things, also for those that needs several required params. As a result — nobody of users that I know about had any problems with understanding what things are required and what are not. We even have it in our features list: "Convenient builders with compile-time guarantees for required params." because it's very easy to misuse API for DB.

Though I'd say that new Retrofit.Builder()....build() is not a very frequent piece of code for a regular app. So it may not worth to do anything to guarantee that all required params are set at compile time. Regular app will crash pretty early with this problem (it's not another DB query in some "hidden" part of the app).

[JakeWharton]

Some libraries base their entire API on that pattern (like jOOQ). It definitely feels far too much machinery for such a low-leverage part of the API, and one that is always exercised like you said.

On Thu, Oct 15, 2015 at 9:43 PM Artem Zinnatullin notifications@github.com wrote:

Another option — add separate Builder class with only one method public CompleteBuilder baseUrl(String baseUrl) so it won't be possible to forget baseUrl and call build(). This option allows you keep almost all pros of regular Builder pattern.

We use this in StorIO (yeah, yeah) for all buildable things, also for those that needs several required params. As a result — nobody of users that I know about had any problems with understanding what things are required and what are not. We even have it in our features list: "Convenient builders with compile-time guarantees for required params." because it's very easy to misuse API for DB.

Though I'd say that new Retrofit.Builder()....build() is not a very frequent piece of code for a regular app. So it may not worth to do anything to guarantee that all required params are set at compile time. Regular app will crash pretty early with this problem (it's not another DB query in some "hidden" part of the app).

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1194#issuecomment-148572053.

[clockworkant]

@JakeWharton :

1. I agree the base url requirement is unlikely to change. I would argue that if a user changes between versions and the builder has a new signature which breaks at compile time this is a good thing. I would rather break at compile time than break at runtime when the builder does a null check and throws because a method has not been called on it such as .baseUrl()
2. I hadn't considered that the documentation in the editor would only display a String as the parameter. I don't think this is a huge issue as developer will need to read the documentation either way. Alternatively this could be avoided by strongly typing the input to the builder but its verbose to make a class to encapsulate a string. new Retrofit.Builder(new BaseUrl("...")) but it does have merit.

@artem-zinnatullin Having a CompleteBuilder is an interesting pattern however I am unsure of the need to have both a Builder and a CompleteBuilder. What circumstances would you choose to use the Builder over the CompleteBuilder?

Overall I still feel having the url passed into the constructor of the builder is the best option. Alternatively passing in a typed string like BaseUrl would also be a second best.

[artem-zinnatullin]

@clockworkant

Having a CompleteBuilder is an interesting pattern however I am unsure of the need to have both a Builder and a CompleteBuilder. What circumstances would you choose to use the Builder over the CompleteBuilder?

You didn't get the idea :)

public static class Builder {

  public CompleteBuilder baseUrl(String baseUrl) {
    return new Builder(baseUrl);
  }

  // it does not have build() method
  // the only methods that it may have — overloads of baseUrl()
}

public static class CompleteBuilder {

  private final String baseUrl;

  // Notice that constructor of CompleteBuilder is not accessible for the user
  CompleteBuilder(String baseUrl) {
    this.baseUrl = baseUrl;
  }

  // other builder methods: addConverterFactory(), etc

  public Retrofit build() {
    return Retrofit(baseUrl, ...);
  }
}

So the usage pattern is to create Builder and then work with CompleteBuilder (it's transparent for the user until he/she wants to store reference to the builder)

new Retrofit.Builder()
  .baseUrl("http://somehost/")
  .addConverterFactory(MoshiConverterFactory.create())
  ....
  .build();

You just can't call build() without setting baseUrl().

@clockworkant constructor and factory method are bad choice in next case: imagine baseUrl becomes non-required and instead of baseUrl scheme (http, https, etc) becomes required. With constructor or factory method, you will have the same signature of the method method(String) so it will be impossible to prevent misusing of the API at compile time.

Another problem is that baseUrl can be passed as String, BaseUrl, HttpUrl — 3 overloads of constructor or factory method. If somewhere in future Retrofit will require another mandatory parameter — constructors and factory methods will become a mess of different combinations of the parameters. Builder pattern solves this problem by design.

But again, for a regular app new Retrofit.Builder() usually needed once, and error caused by incorrect usage of the Builder will be detected early.

[JakeWharton]

I would argue that if a user changes between versions and the builder has a new signature which breaks at compile time this is a good thing.

That would require a major version bump. There's a difference between behavior change and API change, and we don't break APIs between non-major releases for binary compatibility reasons.

[f2prateek]

I'd actually be against this change now, since (correct me if I'm wrong) Retrofit doesn't ship a default converter anymore, so technically, a converter is required.

[f2prateek]

Actually I take that back, I realized you can use RequestBody without a converter.

[JakeWharton]

In practice once is usually required, but technically one is not. You can use RequestBody and ResponseBody types.

[JakeWharton]

I still think your argument holds some merit. If we want to get technical, we could omit a base URL and require absolute URLs on every method annotation.

[hosamaly]

I've found the simple, parameterless constructor useful in allowing me to have a factory method that creates a builder with some default settings, and then handing it over (or back) to another method that would set the more specific arguments, such as the base URL.

[JakeWharton]

We're going to leave this as-is for 2.0.