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

review fix: fix bug with repeatable annotations #1724

Merged
merged 6 commits into from
Nov 14, 2017

Conversation

surli
Copy link
Collaborator

@surli surli commented Nov 14, 2017

fix #1722

@surli surli changed the title WiP fix: Do not factorized annotation when they are repeatable review fix: Do not factorized annotation when they are repeatable Nov 14, 2017
@surli
Copy link
Collaborator Author

surli commented Nov 14, 2017

This PR intends to fix #1722 but it also fixes cases when trying to annotate twice on an annotation type that does not take an Array: the current behavior was to create an array nevertheless, now it throws a SpoonException.

I also had to make a choice when calling repeatedly annotate with a String value on this kind of annotation:

@Repeatable(Tags.class)
public @interface Tag {
    String[] value();
}

We could have indeed those different results:

@Tag("value1")
@Tag("value2")
@Tag("value3")
void foo1();

@Tag({"value1", "value2"})
@Tag("value3")
void foo2();

@Tag({"value1", "value2", "value3"})
void foo3();

Previous behaviour did not take into account the @Repeatable meta-annotation of annotation, so the results corresponding to foo3 were used (then issue #1722), but now with the fix proposed I consider that we should check in priority if the annotation is repeatable or not: if it is, then it creates a new annotation in priority. In this example, foo1 would be created with this new behaviour. foo2 is reachable only if the user decides first to call annotate with an array and then with a single string value.

My guess is that if the user really wants to put an array of objects in the value, then it can always annotate with the array of objects, instead of calling multiple times annotate.

@pvojtechovsky
Copy link
Collaborator

I have checked this PR and it looks good for me. I would merge it but the meaning of word "factorized" in the title is not clear for me. So I am just not sure with merge comment ...

@monperrus monperrus changed the title review fix: Do not factorized annotation when they are repeatable review fix: fix bug with repeatable annotations Nov 14, 2017
@monperrus
Copy link
Collaborator

fixed title. ready for merge!

@pvojtechovsky pvojtechovsky merged commit 0c8c3dc into INRIA:master Nov 14, 2017
@surli surli deleted the fix-repeatable-annotations branch March 26, 2018 10:34
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

Successfully merging this pull request may close these issues.

Unwanted factorized annotation values
3 participants