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.

Sign up for GitHub

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

Jump to bottom

[MNG-8494] Restore Maven 3 compatibility #2031

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

[MNG-8494] Restore Maven 3 compatibility #2031

wants to merge 5 commits into from
+8,071 −6,541

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Jan 7, 2025

No description provided.

elharo
elharo reviewed Jan 8, 2025
View reviewed changes
api/maven-api-model/src/main/mdo/maven.mdo
<codeSegment>
<version>4.0.0/4.0.99</version>
<code>
<![CDATA[
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what CDATA is doing here. It's not needed

api/maven-api-toolchain/src/main/mdo/toolchains.mdo
<codeSegment>
<version>1.0.0/1.1.0</version>
<code>
<![CDATA[
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what CDATA is doing here. It's not needed

...aven-embedder/src/main/java/org/apache/maven/cli/transfer/AbstractMavenTransferListener.java
}

protected AbstractMavenTransferListener(MessageBuilderFactory messageBuilderFactory, PrintWriter out) {
this.messageBuilderFactory = messageBuilderFactory;
protected PrintStream out;
Copy link
Contributor

Choose a reason for hiding this comment

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

PrintStream is wonky for multiple reasons. Can we just use an output stream writer instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fully agree, however the code has already been cleaned up in the non deprecated version:

maven/impl/maven-cli/src/main/java/org/apache/maven/cling/transfer/AbstractMavenTransferListener.java

Line 36 in 698614e

public abstract class AbstractMavenTransferListener extends AbstractTransferListener {

That class is purely for compatibility, so no protected stuff should be changed.

The maven-embedder is here for compatibility and not used when launching Maven from CLI directly.

compat/maven-model-builder/src/main/java/org/apache/maven/model/building/ModelCache.java
/**
* Puts the specified data into the cache.
*
* @param groupId The group id of the cache record, must not be {@code null}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Oracle javadoc guidelines are no initial capitalization on Javadoc comments and period only after or before a complete sentence

compat/maven-model-builder/src/main/java/org/apache/maven/model/building/ModelCacheTag.java

/**
* Creates a copy of the data suitable for retrieval from the cache. The retrieved data can be mutated after the
* cache is queried but the state of the cache must not change so we need to make a copy.
Copy link
Contributor

Choose a reason for hiding this comment

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

queried,

@cstamas cstamas added this to the 4.0.0-rc-3 milestone Jan 8, 2025
@gnodet
Copy link
Contributor Author

gnodet commented Jan 8, 2025

99% of the code here is copy/pasted from maven-3.9.x branch to restore compatibility... so I'm fine with any change you suggest, but I really don't expect you to closely review all the changed files.

@cstamas
Copy link
Member

cstamas commented Jan 8, 2025

These are literally copies of Maven 3 classes.

@cstamas
Copy link
Member

cstamas commented Jan 9, 2025 •
edited
Loading

IMHO last commit should be undone:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elharo elharo elharo left review comments

@cstamas cstamas cstamas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.0.0-rc-3
Development

Successfully merging this pull request may close these issues.
3 participants
@gnodet @cstamas @elharo