jflex
jflex copied to clipboard
jflex-de
Add @Generated annotation on generated code
- Add
@GeneratedFix #453 With this change, all users of JFlex need to havajavax.Generatedin their classpath, for instance by adding a dependency on https://search.maven.org/artifact/javax.annotation/javax.annotation-api/1.3.2/jar - Remove
@SuppressWarningsFix #762 - Also set JVM default Locale to Locale.US to avoid golden diff caused by the number format (Fix #764)
To be honest, seeing the impact it has just on our examples and testing infrastructure, I don't think any more that adding @Generated and requiring a runtime dependency for it is a good idea, at least not as a default.
Would it make sense to provide it as an option instead?
I haven't checked the placement, so this might not work, but could it actually even just go at the end of the user header code as a user-provided annotation?
One of the fall through comments has a typo: https://github.com/jflex-de/jflex/blob/adb1c717014d4b92b9ef284fd2edcf19674adbc7/jflex/src/main/java/jflex/generator/Emitter.java#L1172 (Might it explain the Error Prone errors that were still happening? https://github.com/jflex-de/jflex/issues/222#issuecomment-423757520)
Why not add the warning suppression to the generated methods (e.g. yylex)?
Maybe I'm missing something, but why is @SuppressWarnings("fallthrough") not enough? I don't think requiring users to add a dependency just to ignore some warnings (that they may not have even enabled) is a great idea...
Why not add the warning suppression to the generated methods (e.g. yylex)?
The generated method also contains user code, for which you might want the warning.
I don't think requiring users to add a dependency just to ignore some warnings (that they may not have even enabled) is a great idea...
I agree with that.
The generated method also contains user code, for which you might want the warning.
I thought the idea was to suppress them anyway since it is/was being added to the whole class, but that's a good reason not to.
The generated method also contains user code, for which you might want the warning.
I thought the idea was to suppress them anyway since it is/was being added to the whole class, but that's a good reason not to.
Yes, this PR removes the @SupressWarnings because user-header could perfectly have it too.
And it adds as @Generated annotation instead, which has the same effect for linters, but has no reason to be in user-generated code.
Even though I agree that this is impactful for existing projects, I think it is the proper fix.
@regisd adding @Generated should be handled with care as due to the transaction of JavaEE to JakartaEE the import will change from javax.annotation.Generated to jakarta.annotation.Generated that has to taken into account though. I personally add this annotation within the .jflex file already as well as the @SuppressWarnings
Thank you Patrick. Indeed, I wasn’t aware of this change. More details in the use of Generated in gRoc. https://github.com/grpc/grpc-java/issues/6833
Le mar. 15 déc. 2020 à 10:35, Patrick Reinhart [email protected] a écrit :
@regisd https://github.com/regisd adding @Generated should be handeld with care as due to the transaction of JavaEE to JakartaEE the import will change from javax.annotation.Generated to jakarta.annotation.Generated that has to taken into account though.
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/jflex-de/jflex/pull/763#issuecomment-745168958, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGMVVHEJCYNHRQ3LQO3HJTSU4UUXANCNFSM4M7CJ6JQ .
-- Régis Décamps
http://regis.decamps.info/
At the same time, the existing javax.annotation.Generated will keep existing as it was published, and error-prone will need to keep special treatment for it. I don't expect any new feature on such an annotation. Is there any drawback infusing it, if we it's deprecated?
@regisd the problem is that adding this annotation is not future prove, as the package will change in the future (a major breaking change on the JakarteEE side due to the transition from Oracle to the Eclipse Foundation, the Eclipse guys had to change the namespace). I would prefer to add this annotation by the end user itself not by the generation in the first place, so that I'm able to switch the annotation import as soon as I will need it. Also adding the annotation dependency will break my code in case I want to stay on the old namespace a longer if my customers or other dependencies require it...
@regisd the problem is that adding this annotation is not future prove, as the package will change in the future (a major breaking change on the JakarteEE side due to the transition from Oracle to the Eclipse Foundation, the Eclipse guys had to change the namespace).
Isn't javax.annotation.processing.Generated (which is added in Java 11) the future-proof version?
Isn't javax.annotation.processing.Generated (which is added in Java 11) the future-proof version?
If you use javax.annotation.processing.Generated instead of javax.annotation.Generated for code used in Java 11 and later it should be fine. The only question is if the annotation could be disabled if someone will need to generate code compatible with previous versions...
Closing this in favour of #1027. Users can still add their own @Generated annotation, so there is no need to force an additional runtime dependency on all users, and #764 was already fixed separately in the meantime.