Updating from Scala 2.12 -> 2.13: scala.collection.Seq changes about scrooge HOT 12 CLOSED

Hi @basert, We don't have a flag in Scrooge to generate scala.collection.immutable.Seq from Thrift files. You are welcome to make a PR for this. Internally at Twitter, we haven't upgraded to Scala 2.13 yet.

The scala-collection-compat library can help with cross-building between Scala 2.12 and 2.13. https://github.com/scala/scala-collection-compat

from scrooge.

Sure, we at ESL are more than happy to contribute. Can you maybe point me to some general code guidelines?

For implementation: Should this maybe be the default behavior for Scala 2.13 or would you like to have it behind a flag to keep the current behavior.

from scrooge.

I did some more looking around and it seems basic support for immutable structures was introduced here: 2324adb#diff-addb06caaaca5169adf6c1ff3535cd24b6b3dd5447a88c7718af7aa0ee9b1fadR224

Unfortunately it only mentions for historical reasons as comment to why it's not using collection.immutable.Seq. Would this be the correct place to add the compiler_flag?

from scrooge.

To add another CLI flag, start in the ScroogeOptionParser and add another field to ScroogeConfig for generating immutable Seq.

Yes, the ScalaGenerator.scala is the right place to convert ListType to scala.collection.immutable.Seq. You can use the immutable argument in the genType method to get the immutable collection.

case ListType(x, _) =>
        prefix + "Seq[" + genType(x, immutable).toData + "]"

I believe the "for historical reasons" note refers to backwards compatibility since scala.Seq[A] is an alias for scala.collection.Seq[A] in Scala 2.12.

Should this maybe be the default behavior for Scala 2.13 or would you like to have it behind a flag to keep the current behavior.
I think keeping it behind a flag to maintain the current behavior is the safer choice.

This is the Contributing.md general code guideline https://github.com/twitter/scrooge/blob/develop/CONTRIBUTING.md

Please let me know if you want more guidance navigating the Scrooge codebase.

from scrooge.

Hey, thanks for your guidance.

I did some more digging around and found the language-flags feature. While it seems that it's currently not used anywhere, it is the perfect use for this. I implemented an initial version & added some Gold files to verify the result: #350

Please let me know if it needs some changes or if I should change it to a real scrooge option.

from scrooge.

Hey!

I was able to finish work on the pull request. It requires two changes:

• Add the mentioned language-flag parameter and generate immutable code
• Change TProtocols to explicitly convert the ArrayBuffer into Seq type. This should be a no-op operation for Scala 2.12 and lower because they are of the same type. For Scala 2.13, this will return the immutable instance of Seq.

from scrooge.

Hi @basert , thank you for the contribution! Smart approach! Your code looks great, I especially appreciate all the new tests! To help me understand more about your change, would you mind pointing me to the language-flags feature you mentioned? I don't think anyone is using that internally, so I wanted to understand the impact of this change, this could also help us with passing the knowledge to our internal customers. Thank you again!

from scrooge.

The language-flags is a command line option that forwards a list of strings to the language generator. You can invoke it like this:

java -cp <full-cp> com.twitter.scrooge.Main --finagle --language-flag=immutable-sequences --language scala --dest dist --disable-strict --gen-file-map MyThrift.thrift

We are using it in pants via this buildfile alias:

java_thrift_library(
  compiler='scrooge',
  compiler_args=['--finagle', '--language-flag=immutable-sequences'],
  language='scala',
  strict_deps=True,
  sources=['*.thrift'],
)

from scrooge.

Sounds great! Thank you for elaborating on it!

from scrooge.

Your change in pr#350 has merged, thank you again for your contribution!

from scrooge.

Awesome, this solved the last blocker to upgrade our monorepo to 2.13 🥳

from scrooge.

Thank you for the great work!

from scrooge.