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

Support repeatable annotations #1030

Closed
11 tasks done
Vampire opened this issue Sep 18, 2019 · 4 comments
Closed
11 tasks done

Support repeatable annotations #1030

Vampire opened this issue Sep 18, 2019 · 4 comments

Comments

@Vampire
Copy link
Member

Vampire commented Sep 18, 2019

Groovy 2.5.0 added support for repeatable annotations.
It would be nice if the Spock annotations where it makes sense would be declared repeatable so that you can for example write

@Use(PrivateFinalFieldSetterCategory)
@Use(Whitebox)
def 'my test'() { }

instead of

@Use([PrivateFinalFieldSetterCategory, Whitebox])
def 'my test'() { }

as the former is better readable, better copyable and better commentable individually.

Also for example for Retry it would allow to have different count and delay settings for different exceptions / conditions.

These are the ones I'd say make sense to be repeatable:

  • UseModules
  • Use
  • ConfineMetaClassChanges
  • Subject
  • See
  • Retry
  • Requires
  • PendingFeature
  • PendingFeatureIf
  • Issue
  • IgnoreIf
@leonard84
Copy link
Member

Hm, this would be a breaking change if I'm not mistaken, at least for the non pluralized annotations, the other would need a singular sibling anyway. And for many this would generate a lot more noise IMHO, e.g. I've often seen @StubBeans with 5 or more entries. So having @StubBean 5 plus times is not necessarily better.

@Vampire
Copy link
Member Author

Vampire commented Jan 10, 2020

I don't think it is a breaking change if done properly and you don't necessarily need singular siblings.
If you only have one value you already do not have to use the list syntax, but can simply use @Use(Whitebox), you can just not use mutliple such annotations.

I think you don't need to change the existing annotations at all, besides defining them as @Repeatable.
Then users can either use the annotations like they do now, or use multiple annotations just how they prefer it.
Or e. g. for Retry, you can have 3 exceptions with one delay and 2 other exceptions with another delay by using two annotations.

You then either have to use the AST rewriting to re-combine the annotations back into one for those where it is possible and do not need to change the evaluation logic, but I think the better and more consistent way would be to then where the annotations are evaluated, just request all annotations of the respective type and iterate over them, applying their effect instead of just requesting the sole annotation.

@leonard84
Copy link
Member

Repeatable annotations work by having a separate collector annotation, this cannot be the same as the repeatable annotation itself.

So you would have

@Retention(RetentionPolicy.RUNTIME)
@Repeatable(Annos.class)
@interface Anno {
  String value();
}

@Retention(RetentionPolicy.RUNTIME)
@interface Annos {
  Anno[] value();
}

having a method annotated like this

@Anno('x') @Anno('y')
def foo () {}

will be saved in bytecode like this

@Annos(value={@Anno(value="x"),@Anno(value="y")})
def foo() {}

So you definitely need two separate annotations. And I don't want to write custom AST transformations that won't be recognized by IDEs anyway to work around some of those issues.

@Vampire
Copy link
Member Author

Vampire commented Jan 11, 2020

I'm very well aware of how repeatable annotations work and I didn't deny that you need additional collector annotations.
I just said that you can do this in a backwards compatible way, by not changing more in the existing annotations than making them repeatable and defining their collector annotation and in a flexible way by continuing to support mutliple values to the annotations where it makes sense, but also multiple annotations.

Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 12, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
Vampire added a commit to Vampire/spock that referenced this issue Mar 25, 2020
leonard84 pushed a commit that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 14, 2020
Vampire added a commit to Vampire/spock that referenced this issue Jul 31, 2020
leonard84 pushed a commit that referenced this issue Aug 1, 2020
Vampire added a commit to Vampire/spock that referenced this issue Aug 1, 2020
leonard84 pushed a commit that referenced this issue Aug 1, 2020
Vampire added a commit to Vampire/spock that referenced this issue Aug 1, 2020
leonard84 pushed a commit that referenced this issue Aug 1, 2020
Vampire added a commit to Vampire/spock that referenced this issue Aug 1, 2020
leonard84 pushed a commit that referenced this issue Aug 3, 2020
Vampire added a commit to Vampire/spock that referenced this issue Aug 12, 2020
leonard84 pushed a commit that referenced this issue Aug 13, 2020
Vampire added a commit to Vampire/spock that referenced this issue Aug 13, 2020
leonard84 pushed a commit to leonard84/spock that referenced this issue Aug 21, 2020
Vampire added a commit to Vampire/spock that referenced this issue Nov 1, 2020
Vampire added a commit to Vampire/spock that referenced this issue Nov 1, 2020
Vampire added a commit to Vampire/spock that referenced this issue Nov 1, 2020
Vampire added a commit to Vampire/spock that referenced this issue Nov 1, 2020
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

No branches or pull requests

2 participants