search

julianpeeters / avrohugger

Generate Scala case class definitions from Avro schemas
Apache License 2.0
202 stars 120 forks source link

Mangles names of reserved words #125

Closed withinboredom closed 5 years ago

withinboredom commented 5 years ago

This changes the names of a parameter, such as public into a mangled parameter, such as _$$public.

For case classes: it adds a public val to access it by the name given in the schema For specific classes: the getter and setter should do the translating without changing the schema For scavro: I am honestly not sure how it works :) or if it works :D

There are a few questionable things going on here:

  1. I'm not sure __ is a sufficient mangle or if it would be better to have something else.
  2. The logic smells a bit funny, but not sure if there's a better way without refactoring a bunch of code.
  3. I'm not sure this actually works in real life, for all three cases. Yet. Appears to work as expected, but not sure how to use the scavro classes in a real-life case.

This PR mostly exists to get your feedback on what I have.

withinboredom commented 5 years ago

Seems like there's a bug with compiling the specific records classes. Looking into it.

withinboredom commented 5 years ago

Fixed the compilation issue, I guess the compiler considers names prefixed with _$$ as special.

withinboredom commented 5 years ago

I tested this with our code base and with the julianpeeters/sbt-avrohugger#72 tests added back in.

julianpeeters commented 5 years ago

(1) I'd guess users might expect that the java avro convention was followed, however if something works better for your use case we could use your preference and then in the future add some configurability.

(2) Your concern is very appreciated, but I'm not too worried, avrohugger is ready for a refactor. However I'm strongly opposed to adding a body to the Standard case class format. Could you add or share some scripted tests (e.g. julianpeeters/sbt-avohugger#72) so I can better understand your use-case?

(3) > +publishLocal of a bumped snapshot of avrohugger will make the changes available for a bumped sbt-avrohugger to depend on, from where you can then do > scripted to test scavro etc. in real-world conditions

withinboredom commented 5 years ago

I'd guess users might expect that the java avro convention was followed

Ah, nice! Will reuse that.

However I'm strongly opposed to adding a body to the Standard case class format

I mostly did it because one would expect to be able to call myClass.type (for example) and it just work without having to understand that it's mangled and how it's mangled.

Could you add or share some scripted tests

Will do

withinboredom commented 5 years ago

I changed the mangle function to be the same as the Java version. I also dropped the body from the case class. If it's added, it should probably happen in separately in a different PR.

julianpeeters commented 5 years ago

Awesome, thank you @withinboredom! This is now out as RC18

Regarding the on the body -- I've got to get the testing situation in better control before new custom type options, or new formats, are added. I'd be looking for a more specific use case than reflection, tho (reflection is currently labelled "experimental", and in general is infeasible for avrohugger to support over time)