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

Include generic parameters in record name #444

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

charlibot
Copy link

Fixes "Generic parameters are not included in a record name" of #442.

Follows the same conventions as Avro4s by separating the case class name and types with __ and the types themselves with _.

@bplommer
Copy link
Member

bplommer commented May 5, 2022

I like this but I'd like to make it configurable so we don't make breaking changes to existing code - it would also be good to have configurable separators I think.

We could either add a version of derive that takes configuration arguments or add a new annotation AvroNameWithParamNames(name: String, sep1: String = "__", sep2: String = "_") - I lean to the first option, what do you think?

I'm also open to better naming suggestions 🙂

@charlibot
Copy link
Author

Thanks for the feedback @bplommer - I also prefer the first option of passing configuration parameters to the derive method. Will try to get that working in the PR.

@bplommer
Copy link
Member

bplommer commented May 6, 2022

One thing I'd say is to use a builder rather than a case class for the config so that it can be extended later in a binary compatible way. Not sure how familiar you are with that pattern, happy to go into more detail if needed.

@charlibot
Copy link
Author

@bplommer - I have something working for Scala 2 and am a little stuck on Scala 3. Before getting in too deep, I wanted to check with you I'm heading in the right direction.

I've added a Configuration case class with withTypeSeparators as a sort of builder method; I was taking inspiration from circe-generic-extra's config class. Then this is wired in as an implicit parameter.

@erikvanoosten
Copy link
Contributor

@bplommer Are you sure you want to make the separators configurable? What kind of use case do you have in mind? It seems to need a lot of code so we should be sure it is worth it.

@eugkhp
Copy link

eugkhp commented Nov 9, 2023

Hey! @bplommer any update on this, is it possible to merge it? I'm also migrating from avro4s and this would be nice to have

@eugkhp
Copy link

eugkhp commented Nov 10, 2023

I think I found the solution how to do it without modifying the libarary

  final case class Event[A: Codec](
      eventId: EventId,
      eventTimestamp: Instant,
      data: A,
    )

  object Event {
    implicit def codec[A: Codec]: Codec[Event[A]] =
      Codec.record[Event[A]](
        s"${Codec[A].schema.map(sc => s"Event__${sc.getName}").getOrElse("couldn't derive avro schema for A")}",
        s"${Codec[A].schema.map(_.getNamespace).getOrElse("couldn't derive avro schema for A")}",
      )(builder =>
        (
          builder("eventId", _.eventId),
          builder("eventTimestamp", _.eventTimestamp),
          builder("data", _.data),
        ).mapN(Event(_, _, _))
      )
  }

but this won't solve anything nested ofc 😔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants