sbt / sbt-native-packager

sbt Native Packager
https://sbt-native-packager.readthedocs.io/en/stable/
BSD 2-Clause "Simplified" License
1.6k stars 441 forks source link

Question regarding documentation (publishing settings) #1363

Open kkalavantavanich opened 4 years ago

kkalavantavanich commented 4 years ago

I was looking at the documentation here: https://www.scala-sbt.org/sbt-native-packager/formats/docker.html#publishing-settings. The documentation provides this description about dockerAliases:

The list of aliases to be used for tagging the resulting image of the Docker build. The type of the setting key is Seq[DockerAlias]. Alias values are in format of [dockerRepository/][dockerUsername/][packageName]:[tag] where tags are list of including your project version and latest tag(if dockerUpdateLatest is enabled). To append additional aliases to this list, you can add them by extending dockerAlias.

dockerAliases ++= Seq(dockerAlias.value.withTag(Option("stable")), dockerAlias.value.withRegistryHost(Option("registry.internal.yourdomain.com")))

My questions are:

  1. Is there any particular reasons to use Option and not Some?
  2. Why is this even an Option? Wouldn't just using the value provide a better API?
dockerAliases ++= Seq(dockerAlias.value.withTag("stable"), dockerAlias.value.withRegistryHost("registry.internal.yourdomain.com"))
muuki88 commented 4 years ago

Hi @kkalavantavanich

thanks for your question regarding this :smile:

Is there any particular reasons to use Option and not Some?

I found this is quite a few places as a good practice due to two reasons

  1. type inference works correctly in some cases
  2. prevents accidentally passing null. Maybe not in this case. However image refactoring this into a method that reads a file and you parse some content. I'm saying as soon as you rely on some java apis this breaks

Why is this even an Option? Wouldn't just using the value provide a better API?

I agree on this one. It should have probably been to methods

def withTag(tag: String): DockerAlias = copy(tag = Some(tag))
def withNoTag(): DockerAlias = copy(tag = None)

If it's possible to enhance this in a binary compatible way, I'm open for pull requests :smiling_face_with_three_hearts: