SPICE-0024: Annotation converters

[HT154]

No description provided.

[stackoverflow]

You mentioned you didn't want an implicit ConverterAnnotation and I agree it's better to be explicit. But why not provide it just for usability sake? No one would be forced to use it, but I imagine many people would rather use a class pre-made for this use case than remember what the api is supposed to be like.
That would make it easier to remember what a converter is supposed to do, and the converter code would be:

annotationConverters {
  [SnakeCase] = (name, annotation, value) -> annotation.convert(name, value)
}

It could also have methods that only take the property name or just the value. So you can choose what to overwrite

abstract class ConverterAnnotation extends Annotation {
  function convert(propertyName: String, value: Any): Pair<String, Any> =
    Pair(convertName(propertyName), convertValue(value))

  function convertName(propertyName: String): String = propertyName

  function convertValue(value: Any): Any = value
}

You would still be able to write your own annotation and call it however you please.

[HT154]

Are you proposing that renderers implicitly convert subclasses of ConverterAnnotation by default? Or would users still need to explicitly add the converter?

I would be opposed to adding an implicit annotation converter for ConverterAnnotation (for the reasons outlined in the SPICE), but open to adding this type for explicit use if y'all feel we should.
One reason to omit this is that it does increase the surface area of pkl:base for a type that is basically sugar.

[stackoverflow]

Are you proposing that renderers implicitly convert subclasses of ConverterAnnotation by default?

No. It all explicit, as you proposed.
You still need to wire the converters in the renderer body.
This class would just be there as a template that you can (but don't have to) use, so you can:

class RenderDuration extends ConverterAnnotation {
  unit: DurationUnit

  function convertValue(value: Duration): Number = value.toUnit(unit).value
}

output {
  renderer {
    annotationConverters {
      [RenderDuration] = (prop, annotation, value) -> annotation.convert(prop, value)
    }
  }
}

[bioball]

Creating a new class in the base module is a breaking change to how name resolution works.
So, I'd rather be conservative and avoid adding new classes for this (unless we want to put this in another stdlib module).

[stackoverflow]

Yeah, putting this on base is a bad idea. And we can always add it later.

[bioball]

At some point, it might be good to add some way to control what an annotation can be applied to.

This avoids mistakes like applying @SnakeCase to a class, typealias, etc.

[bioball]

Do we ever care to support annotation converters on something that's not a property?

Right now, this isn't meaningful, but it might be if we ever support annotations on other source code locations.

foo {
  @MyAnnotation
  new { bar =1 }
}

Thinking through this: I don't think it's likely that we'd support adding annotations to object members. So, my own conclusion here is that... no, we don't care for it.

[bioball]

The order of parameters here seems strange. Why is Annotation in between the key and the value?

I think it should be:

Suggested change

	annotationConverters: Mapping<Class, (String, Annotation, unknown) -> Pair<String, Any>>
	annotationConverters: Mapping<Class, (Annotation, String, unknown) -> Pair<String, Any>>

[bioball]

Something else to mention here is: annotation converters should eventually influence parsing, too.
This way, you can round-trip between Pkl and some external format.

The story is somewhat muddy right now because we don't have any way to parse into a Typed value.
But, once we have that, a Parser should also support annotation converters, where the converter goes the opposite direction.