Proposed @NamedVariant breaking change edge case

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Proposed @NamedVariant breaking change edge case

paulk_asert

Hi folks,

I have been thinking about:


If you remember the design behind @NamedVariant, there were
two schools of thought. One group were keen on always having the first
parameter as a delegate; it's properties determining the keys for named args.
Another group were keen on the names of the params themselves
being the keys for named args. Our current design using @NamedParam
and @NamedDelegate market annotations covers both these use cases
and even allows mix and match.

I don't plan on changing anything in cases where either @NamedParam
or @NamedDelegate are used, however, I think I made the wrong call for
what should happen if no marker annotations are placed on the parameters.
I went with defaulting the behavior as if @NamedDelegate was implicitly
added to the first param. In fact, I think it should have been as if
@NamedParam was added to all parameters.

Basically, I propose making this (using the GROOVY-8703 example):

 @NamedVariant
  Color(Integer r, Integer g, Integer b) {
    this.r = r; this.g = g; this.b = b
  }

Be the same as this:

 @NamedVariant
  Color(@NamedParam Integer r, @NamedParam Integer g, @NamedParam Integer b) {
    this.r = r; this.g = g; this.b = b
  }

Given 2.5 has only been out for a few months, I suspect @NamedVariant
uptake won't be huge at this point and most probably folks aren't relying on
the behavior when the additional marker annotations are left off.
I propose we change this behavior and list it as a breaking change in
the 2.5.3 release notes. If we delay any longer though, I think we will need
to stick with the current design.

We could add an autoDelegate annotation attribute to allow the old behavior
to be switched on but I think in fact using @NamedDelegate on the first
parameter is actually clearer anyway. I guess for annotation collector scenarios
something like autoDelegate might be useful.

Thoughts?

Cheers, Paul.

MG
Reply | Threaded
Open this post in threaded view
|

Re: Proposed @NamedVariant breaking change edge case

MG
I think you are right, and having only @NamedVariant on a method should equal also having @NamedParam on every parameter. 

Rationale: I feel it is the more common case & least surprise, adding @NamedParam to all arguments is cumbersome and creates a lot of clutter/noise, and (as you said) making the @NamedDelegate case explicit is a good idea.

+1 on changing this.


-------- Ursprüngliche Nachricht --------
Von: Paul King <[hidden email]>
Datum: 04.09.18 10:17 (GMT+01:00)
Betreff: Proposed @NamedVariant breaking change edge case


Hi folks,

I have been thinking about:


If you remember the design behind @NamedVariant, there were
two schools of thought. One group were keen on always having the first
parameter as a delegate; it's properties determining the keys for named args.
Another group were keen on the names of the params themselves
being the keys for named args. Our current design using @NamedParam
and @NamedDelegate market annotations covers both these use cases
and even allows mix and match.

I don't plan on changing anything in cases where either @NamedParam
or @NamedDelegate are used, however, I think I made the wrong call for
what should happen if no marker annotations are placed on the parameters.
I went with defaulting the behavior as if @NamedDelegate was implicitly
added to the first param. In fact, I think it should have been as if
@NamedParam was added to all parameters.

Basically, I propose making this (using the GROOVY-8703 example):

 @NamedVariant
  Color(Integer r, Integer g, Integer b) {
    this.r = r; this.g = g; this.b = b
  }

Be the same as this:

 @NamedVariant
  Color(@NamedParam Integer r, @NamedParam Integer g, @NamedParam Integer b) {
    this.r = r; this.g = g; this.b = b
  }

Given 2.5 has only been out for a few months, I suspect @NamedVariant
uptake won't be huge at this point and most probably folks aren't relying on
the behavior when the additional marker annotations are left off.
I propose we change this behavior and list it as a breaking change in
the 2.5.3 release notes. If we delay any longer though, I think we will need
to stick with the current design.

We could add an autoDelegate annotation attribute to allow the old behavior
to be switched on but I think in fact using @NamedDelegate on the first
parameter is actually clearer anyway. I guess for annotation collector scenarios
something like autoDelegate might be useful.

Thoughts?

Cheers, Paul.

Reply | Threaded
Open this post in threaded view
|

Re: Proposed @NamedVariant breaking change edge case

Jochen Theodorou
In reply to this post by paulk_asert


Am 04.09.2018 um 10:17 schrieb Paul King:
[...]

> Basically, I propose making this (using the GROOVY-8703 example):
>
>   @NamedVariant
>    Color(Integer  r,Integer  g,Integer  b) {
>      this.r = r;this.g = g;this.b = b
>    }
>
>
> Be the same as this:
>
>   @NamedVariant
>    Color(@NamedParam Integer r, @NamedParam Integer g, @NamedParam Integer b) {
>
>      this.r = r;this.g = g;this.b = b
>    }

reminds me a lot of the code I just saw for the definition of a rest
endpoint in spring with annotations for about everything.

Can't the usage of @NamedParam imply @NamedVariant?

> Given 2.5 has only been out for a few months, I suspect @NamedVariant
> uptake won't be huge at this point and most probably folks aren't relying on
> the behavior when the additional marker annotations are left off.
> I propose we change this behavior and list it as a breaking change in
> the 2.5.3 release notes. If we delay any longer though, I think we will need
> to stick with the current design.

I think we should work more with marking new features as experimental

  bye Jochen
Reply | Threaded
Open this post in threaded view
|

Re: Proposed @NamedVariant breaking change edge case

paulk_asert


On Wed, Sep 5, 2018 at 2:56 AM Jochen Theodorou <[hidden email]> wrote:


Am 04.09.2018 um 10:17 schrieb Paul King:
[...]
> Basically, I propose making this (using the GROOVY-8703 example):
>
>   @NamedVariant
>    Color(Integer  r,Integer  g,Integer  b) {
>      this.r = r;this.g = g;this.b = b
>    }
>
>
> Be the same as this:
>
>   @NamedVariant
>    Color(@NamedParam Integer r, @NamedParam Integer g, @NamedParam Integer b) {
>
>      this.r = r;this.g = g;this.b = b
>    }

reminds me a lot of the code I just saw for the definition of a rest
endpoint in spring with annotations for about everything.

Can't the usage of @NamedParam imply @NamedVariant?

You could do this but currently @NamedParam and @NamedDelegate are marker
interfaces so you'd need a global transform.
 
> Given 2.5 has only been out for a few months, I suspect @NamedVariant
> uptake won't be huge at this point and most probably folks aren't relying on
> the behavior when the additional marker annotations are left off.
> I propose we change this behavior and list it as a breaking change in
> the 2.5.3 release notes. If we delay any longer though, I think we will need
> to stick with the current design.

I think we should work more with marking new features as experimental

That would have been a great option (in hindsight) here. :-)
 

  bye Jochen