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

check "--enable-preview" flag in ${maven.compiler.enablePreview} #1555

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

RussiaVk
Copy link
Contributor

check "--enable-preview" flag in ${maven.compiler.enablePreview}:
```<properties>
	<maven.compiler.enablePreview>true</maven.compiler.enablePreview>
</properties>
```

Copy link
Contributor

@HannesWell HannesWell left a 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.

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Test Results

107 files  ±0  107 suites  ±0   11m 33s ⏱️ + 1m 39s
662 tests +1  652 ✔️ +1  10 💤 ±0  0 ±0 
662 runs  +1  651 ✔️ +1  11 💤 ±0  0 ±0 

Results for commit d206d5c. ± Comparison against base commit 8fe2220.

♻️ This comment has been updated with latest results.

@RussiaVk
Copy link
Contributor Author

RussiaVk commented Oct 8, 2023

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.

Hello, could you please assist me in reviewing the code and kindly point out any errors if they are present?

Copy link
Contributor

@HannesWell HannesWell left a 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):

@RussiaVk
Copy link
Contributor Author

RussiaVk commented Oct 9, 2023

Please see the comments about the code blow.

Furthermore you still need to electronically sign the Eclipse Contributor Agreement (ECA):

There seems to be an error in jenkins, how to fix it?

@mickaelistria
Copy link
Contributor

ECA signature seems fine now.
The remaining issue that is probably related to this change is https://ci.eclipse.org/m2e/job/m2e/job/PR-1555/19/testReport/org.eclipse.m2e.jdt.tests/JavaConfigurationTest/testComplianceVsEnablePreviewSettings/

Copy link
Contributor

@mickaelistria mickaelistria left a 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"

Copy link
Contributor

@HannesWell HannesWell left a 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}
@HannesWell HannesWell merged commit c9c717c into eclipse-m2e:master Oct 10, 2023
@HannesWell HannesWell added this to the 2.5.0 milestone Nov 27, 2023
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.

3 participants