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

Fix discouraged access warnings for own code #1206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bananeweizen
Copy link
Contributor

Have all m2e project accesses be silenced by exporting the related packages via x-friends. That's safe since all that code is changed and released together.

Disable discouraged access warnings in test projects. Tests can be adapted the moment that referenced class vanishes, there is no need to require stable APIs only in tests.

Finally remove no longer needed SuppressWarnings.

@github-actions
Copy link

github-actions bot commented Jan 14, 2023

Test Results

   196 files  ±0     196 suites  ±0   23m 23s ⏱️ + 3m 14s
   611 tests ±0     602 ✔️  - 1    7 💤 ±0  2 +1 
1 222 runs  ±0  1 206 ✔️  - 1  14 💤 ±0  2 +1 

For more details on these failures, see this check.

Results for commit 83f36ba. ± Comparison against base commit 7943365.

♻️ This comment has been updated with latest results.

@Bananeweizen Bananeweizen force-pushed the fix_restricted_access_warnings branch from aa88bf3 to 0b8dd69 Compare January 15, 2023 09:20
Have all m2e project accesses be silenced by exporting the related
packages via x-friends. That's safe since all that code is changed and
released together.

Disable discouraged access warnings in test projects. Tests can be
adapted the moment that referenced class vanishes, there is no need to
require stable APIs only in tests.

Finally remove no longer needed SuppressWarnings.
@HannesWell HannesWell force-pushed the fix_restricted_access_warnings branch from 0b8dd69 to 83f36ba Compare January 16, 2023 18:17
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 in general, only one point in the comments.

Comment on lines +14 to +16
org.eclipse.m2e.jdt.ui,
org.eclipse.m2e.tests,
org.eclipse.m2e.editor.tests",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it is a bit strange, to mark 'test-plugins' as x-friends. Can't you instead disbale discouraged access-warnings in the preferences of the tests, just like you did in the apt.tests?

Applies to other 'production' plugins too.

Export-Package: org.eclipse.m2e.tests.common;x-friends:="org.eclipse.m2e.tests,org.eclipse.m2e.core.tests,org.eclipse.m2e.jdt.tests,org.eclipse.m2e.core.ui.tests,org.eclipse.m2e.wtp.tests"
Export-Package: org.eclipse.m2e.tests.common;
x-friends:="org.eclipse.m2e.tests,
org.eclipse.m2e.core.tests,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this plug-in itself is only for tests, I think it would be ok to define other test-plugins as friends. But if you disable discouraged access warnings there that is probably not necessary.

Copy link
Contributor Author

@Bananeweizen Bananeweizen Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, will disable it in all test bundles, and then apply only the remaining fixes in non test bundles. Since the bundles currently have very mixed preferences (or no preferences) this might require to first create preferences in all of them. Otherwise search/replace leads to such confusing results.

@HannesWell
Copy link
Contributor

@Bananeweizen do you want to complete this one?

@akurtakov
Copy link
Contributor

What is the status here?

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