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

Mark generated classes as generated #116 #117

Merged
merged 1 commit into from Sep 10, 2019
Merged

Mark generated classes as generated #116 #117

merged 1 commit into from Sep 10, 2019

Conversation

Serranya
Copy link
Contributor

@Serranya Serranya commented Aug 2, 2019

@jjlauer this is my initial attempt at #116.

I haven't made the feature configurable yet, because there is something I want to discuss first.

My intention was to apply the annotation to every class rocker generates. The catch is, that rocker generates:

  • anonymous classes for java 6 and 7
  • lambas for java 8

Right now java does not allow annotations on either of them.

For anonymous classes there might be a solution in the form of JSR308 (Introduced in java 8).

callMethod(new @Generated AnonymousClass(){})

But this also has it's problems:

  1. I don't think JaCoCo supports JSR308 annotations.
  2. This only works for anonymous classes. Those are only generated by rocker on the <8 java profile. But JSR308 requires at least java 8.

Those are the options we have in the order my preference:

0: Promote the anonymous classes and lambdas to proper classes
Let's generate the problematic classes as static inner classes that can be annotated.

Pros:

  • Only rocker code changes required.
  • Completely solves the problem. JaCoCo works now.

Cons:

  • This is a rather big change.

1: Ask JaCoCo to change
We/I can ask JaCoCO to change their logic. They should ignore anonymous classes or lambdas inside of classes marked as @Generated.

Pros:

  • The rocker changes are kept minimal.
  • Seems like the right thing to me.

Cons:

  • Bigger coordination effort since 2 projects are now affected.

2: Abandon the idea
Let's do nothing and chill.

Pros:

  • Still better than 3 imo.

Cons:

  • My problem is not solved :(

3: Use JSR308 annotations
For java8 let's use JSR308 annotations. Remove the lambdas and replace them with anonymous classes.

Pros:

  • ?

Cons:

  • Changes in JaCoCo required.
  • Only works for java >8.
  • Combines the disadvantage of a big change as in 0, with none of it's advantages.

@Serranya
Copy link
Contributor Author

Serranya commented Aug 2, 2019

Actually I am an idiot. JaCoCo already works the way I suggested in 1. Therefor you can ignore my wall of text above.

@Serranya Serranya marked this pull request as ready for review August 2, 2019 08:55
@Serranya
Copy link
Contributor Author

Serranya commented Aug 7, 2019

@jjlauer have you had a chance to take a look at this PR?

@jjlauer
Copy link
Member

jjlauer commented Aug 7, 2019

Apologize for the delay on my end. This looks great! Nice work.

@jjlauer
Copy link
Member

jjlauer commented Aug 7, 2019

Would you by chance include a few sentences to the README so folks know about this feature?

@Serranya
Copy link
Contributor Author

Serranya commented Aug 7, 2019

Sure. I will add them

@Serranya
Copy link
Contributor Author

@jjlauer I updated the README

@Serranya
Copy link
Contributor Author

@jjlauer ping

1 similar comment
@Serranya
Copy link
Contributor Author

Serranya commented Sep 2, 2019

@jjlauer ping

@Serranya
Copy link
Contributor Author

Serranya commented Sep 9, 2019

@jjlauer is there anything else I have to do to get this PR accepted?

@jjlauer jjlauer merged commit c6ba7c3 into fizzed:master Sep 10, 2019
@jjlauer
Copy link
Member

jjlauer commented Sep 10, 2019

Merged the PR. Also released v1.2.2 for ya.

@Serranya Serranya deleted the generated branch September 10, 2019 07:32
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.

None yet

2 participants