-
Notifications
You must be signed in to change notification settings - Fork 118
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
check "--enable-preview" flag in ${maven.compiler.enablePreview} #1555
Conversation
RussiaVk
commented
Sep 19, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution.
Before we can accept it you have to (electronically) sign the Eclipse Contributor Agreement (ECA):
https://www.eclipse.org/legal/ecafaq.php
In general it looks good. Just one required adjustments below.
Futhermore we need a test-case to demonstrate that this new feature works now and in the future. Can you please add a little case in org.eclipse.m2e.jdt.tests.JavaConfigurationTest
that has the new enablePreview
property set. Ideally also test cases for the first and second way to enable preview features would be added, because I haven't found any.
Creating them should be relatively simple as it was done e.g. in 75f25ab.
org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/AbstractJavaProjectConfigurator.java
Outdated
Show resolved
Hide resolved
Hello, could you please assist me in reviewing the code and kindly point out any errors if they are present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the comments about the code blow.
Furthermore you still need to electronically sign the Eclipse Contributor Agreement (ECA):
org.eclipse.m2e.jdt.tests/projects/compilerEnablePreviewSettings/pom.xml
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JavaConfigurationTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JavaConfigurationTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JavaConfigurationTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JavaConfigurationTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JavaConfigurationTest.java
Outdated
Show resolved
Hide resolved
There seems to be an error in jenkins, how to fix it? |
ECA signature seems fine now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me.
Reminder to whoever will merge: please use "Squash and Merge"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me as well.
Thank you for this nice contribution, it will be part of the 2023-12 Eclipse Simulations release.
check --enable-preview flag in ${maven.compiler.enablePreview}