nspl icon indicating copy to clipboard operation
nspl copied to clipboard

Parameters

Open SeZuo opened this issue 2 years ago • 3 comments

Hey,

Any reason trait SimplePlots's class Parameters() is not a simple case class Params() ?

It seems you would benefit from the case class copy function, and Parameters does not need to be called by reference (at least I couldn't find where it would).

Not really an issue, just curious of this factory pattern.

Thanks in advance for your answer.

SeZuo avatar Apr 30 '22 16:04 SeZuo

It looks like a case class would work. I think I wanted to decouple the name of the class from the name of the type. This decision was probably made before that list was much shorter..

pityka avatar Apr 30 '22 17:04 pityka

Thanks again for your answer.

Tried with a case class and indeed, no error running the example.

You can use a type alias and a function alias for instantiation.

type par = Parameters
  def par = Parameters

Although, I looked into it because I didn't understand why it was par (function because of lower case, probably short for "parallel" in my mind), and not Params.

We can close for me, unless you want a PR for that.

SeZuo avatar Apr 30 '22 18:04 SeZuo

the name par mimics this: https://www.rdocumentation.org/packages/graphics/versions/3.6.2/topics/par, indeed probably does not ring a bell for many.

I believe the only difference is that the compiler will synthesize a big unapply for the case class.

In terms of binary compatibility both are bad options, the only way out I know would be to use a builder like pattern and separate all those options into their own methods (withThis withThat). I did not opt for that because that is longer to type at the call site.

In short, if you thing it is better with case class and send a PR I will merge it.

pityka avatar Apr 30 '22 18:04 pityka

@pityka Here is an updated page on how to create binary compatible case classes: https://docs.scala-lang.org/overviews/core/binary-compatibility-for-library-authors.html

Scroll down to the section entitled "Evolving code without breaking binary compatibility" and there are examples of creating "withXXX"-methods, private unapply, etc.

bjornregnell avatar Jan 23 '23 15:01 bjornregnell

Hello @bjornregnell! Thanks for the pointers.

I am contemplating on saving the keystrokes of the 'with` and omitting that prefix, e.g.

case class Person private (name: String, age: Int):
  // Create withXxx methods for every field, implemented by using the copy method
  def name(name: String): Person = copy(name = name)
  def age(age: Int): Person = copy(age = age)

Do you think that will be OK for usability? I would be happy to have your feedback. Thanks!

pityka avatar Jan 28 '23 20:01 pityka

Happy to help @pityka ! I think that the overloaded usage of the name name might be a bit confusing as it is a method name and a field name and a parameter (to the name method), so I think I prefer the idiom of withXXX even if it is a few more characters to type. This also has the nice effect that you can start typing with and your IDE might help you suggest completions to help you discover all available with-methods, which is nice for usability of the api and you can press TAB or similar depending on your IDE. Also with signals a copy for those used to this idiom. (But I can live without the with also if you prefer that and it is also nice to be concise...)

bjornregnell avatar Jan 28 '23 21:01 bjornregnell

Thanks for the explanations @bjornregnell . I agree that discoverability in an IDE is important in this case. In PR #121 I added two copy methods per parameter: e.g. for xlog:

case class Parameters private (xlog: Boolean) {
  def xlog(v:Boolean) = copy(xlog = v)
  def withXLog(v:Boolean) = copy(xlog = v)
}
object Parameters {
  @scala.annotation.nowarn
  private def unapply(p: Parameters) = p
  def apply() = new Parameters(xlog = false)
}

So one can write the with prefix and ask an IDE for completion, or one can directly write the name of the respective method.

pityka avatar Feb 06 '23 08:02 pityka

Great! Adding both gives the best of both worlds (albeit at the expense of a bigger api). I think it would be good to explain the rationale behind this in the api docs.

bjornregnell avatar Feb 07 '23 16:02 bjornregnell