Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add collection_type settings #218

Closed
wants to merge 1 commit into from

Conversation

xuwei-k
Copy link
Contributor

@xuwei-k xuwei-k commented Dec 23, 2016

https://gitter.im/trueaccord/ScalaPB?at=583dc53cb4d7ca3b7a1810fa

Hello. Is there a way to generate List instead of Seq for repeated? I wrote a library that can convert between normal domain case classes and scalaPB generated case classes where all fields are optional. https://github.com/kailuowang/henkan#transform-between-case-classes-with-optional-field, however for repeated items, I need cats Functor and Traverse instances. There are no such instances for Seq provided out of the box.

/cc @kailuowang

Edit: also fix #208

@kailuowang
Copy link

This is awesome! Thanks a lot! @xuwei-k

import protocbridge.JvmGenerator

package object scalapb {
def gen(
flatPackage: Boolean = false,
javaConversions: Boolean = false,
grpc: Boolean = true,
singleLineToString: Boolean = false): (JvmGenerator, Seq[String]) =
singleLineToString: Boolean = false,
collectionType: String = "scala.collection.Seq"): (JvmGenerator, Seq[String]) =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that this would be a field-level setting (in scalapb.proto), not a global compiler setting. The use case I see is that a single repeated field in a proto is known to be a set, while the rest of the file stays with the default implementation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A field level set is definitely a good use case but for our projects being able to set a global collection type for repeated items different than Seq would be really helpful. Seq isnt very strict so that the functional programming libs we uses don't support it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do a file-level option then. We found that global compiler settings (such as flat_package) that affect the public API of the generated code hurts maintainability issues down the line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A set of three options would be the best in my opinion.
In some cases I would really like to use plain arrays of primitives for the performance reasons, for example.

So, options having priorities from high to low in the following order would be really the perfect variant:
Field level option -> File-level option -> Global option -> Seq

@hseeberger
Copy link

What's the status of this PR? We urgently need to be able to use immutable sequences.

@thesamet
Copy link
Contributor

I looked into it today, started to make some changes and it's currently parking in the collection_type. The main difference is that it's going to have a file and field-level options, but not as a global option. Global options should not affect the generated interfaces (I made that mistake with flat_package already) - it makes it impossible for other generators to know what interface to expect.

The current issues that still need to be solved:

  • The lenses library has something called SeqLens that only works with Seqs. It needs to be parameterized over the collection type.
  • getField() and fromFieldMap are used by TextFormat and the json conversion library. Those assume that scala.collection.Seq are passed for repeatables and they do asInstanceOf[$collectionType], which would throw at runtime (for things like Array and Vector, but not immutable.Seq). Scala descriptors (coming soon) will have a type-safe version of getField and fromFieldsMap and TextFormat and JSON are ported to work with them. It will be a better time to potentially break these two methods.
  • Minor: Some collection types won't work with Java conversions - Array doesn't have asJava, probably not a big deal.

@thesamet
Copy link
Contributor

Thanks for the PR @xuwei-k . Committed 5ce86e9 which is based on your work.

@thesamet thesamet closed this Feb 20, 2017
@xuwei-k xuwei-k deleted the collection-type branch February 20, 2017 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to generated repeated fields as Sets
5 participants