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

W-17457827: JPMS support for the RevAPI plugin. #172

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

IvanAndresFritzler
Copy link

No description provided.

@IvanAndresFritzler IvanAndresFritzler requested a review from a team as a code owner December 27, 2024 18:46
fsgonz
fsgonz previously approved these changes Dec 30, 2024
Copy link

@fsgonz fsgonz left a comment

Choose a reason for hiding this comment

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

LGTM in general.

Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 52.6% Line Coverage on New Code (is less than 80%)

See analysis details on SonarQube

Set<ExportedPackages> exportedPackages = new HashSet<>();
api.getArchives().forEach(archive -> {
if (!addJavaModuleSystemExportedPackages(api, archive, exportedPackages)) {
if (!addMuleModuleSystemExportedPackages(archive, exportedPackages)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use && instead of nested ifs

}

private boolean addMuleModuleSystemExportedPackages(Archive archive, Set<ExportedPackages> exportedPackages) {
if (isMixedMode() || isMuleMode()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't isMuleMode checked for mixed mode as well?

}

private boolean addJavaModuleSystemExportedPackages(API api, Archive archive, Set<ExportedPackages> exportedPackages) {
if (isJavaMode() || isMixedMode()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't isJavaMode checked for mixed mode as well?


boolean isExported(String packageName);

void logExportedPackages();
Copy link
Contributor

Choose a reason for hiding this comment

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

why this is weird... by the name this seems to be a value object... why does it have logging behaviour?


public interface ExportedPackages {

boolean isExported(String packageName);
Copy link
Contributor

Choose a reason for hiding this comment

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

add javadocs

if (moduleReferences.isEmpty()) {
return empty();
} else {
return Optional.of(moduleReferences.iterator().next());
Copy link
Contributor

Choose a reason for hiding this comment

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

static import

Comment on lines +67 to +69
Field archiveFile = archive.getClass().getDeclaredField("file");
archiveFile.setAccessible(true);
return ((File) archiveFile.get(archive)).toPath();
Copy link
Contributor

Choose a reason for hiding this comment

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

archive.getName()?

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