8000 extract common functionality of DefaultPluginXmlFactory by Pankraz76 · Pull Request #2326 · apache/maven · GitHub
[go: up one dir, main page]
Skip to content

extract common functionality of DefaultPluginXmlFactory #2326

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

extract common functionality of DefaultPluginXmlFactory #2326

wants to merge 1 commit into from

Conversation

Pankraz76
Copy link
Contributor
@Pankraz76 Pankraz76 commented May 12, 2025 •
edited
Loading

This was referenced May 12, 2025
Merged
Open
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from c99650c to b9ac82d Compare May 12, 2025 15:05
@Pankraz76 Pankraz76 marked this pull request as ready for review May 12, 2025 15:07
gnodet
gnodet requested changes May 12, 2025
View reviewed changes
Copy link
Contributor
@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

It really does not make any sense to me to wrap the request (immutable data object) into another object, just for the sake of having methods collocated with the data.

@Pankraz76
Copy link
Contributor Author
Pankraz76 commented May 12, 2025 •
edited
Loading

https://refactoring.guru/design-patterns/factory-method

Considering factory adhering to creational design pattern its supposed to create stuff, handling the what concern, like shown in pr, not to actually implement and fulfill the doing, being the how concern.

sake of having methods collocated with the data.

yes this is considered OOP, IoC, SRP, and SOC avoiding the feature envy. Our factory is actually an worker which is considered two different kind. Might be related but is not same or equal.

Grouping data and its entourage; companion into dedicated classified dimensions.

When this factory is handling 100 products im sure they wont all gather inside on place.

@Pankraz76
Copy link
Contributor Author

It really does not make any sense to me to wrap the request (immutable data object) into another object, just for the sake of having methods collocated with the data.

we can inline of course and least separate code on impl lvl giving each dedicated concern.

Imho considering SOLID one thing for factory is to delegate and create, thats more then enough.

Pankraz76
Pankraz76 commented May 12, 2025
View reviewed changes
Copy link
Contributor Author
@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

pointing out dedicated concerns.

Raising question about factory name and design as still factory is only creational pattern, having to delegate like everybody else following SOLID.

@Pankraz76
Copy link
Contributor Author

sacrifice/spike architectural design again, in sake to increment code.

@Pankraz76 Pankraz76 changed the title Resolve feature envy in DefaultPluginXmlFactory Cure feature envy of DefaultPluginXmlFactory May 13, 2025
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 5b450eb to 0d2a242 Compare May 13, 2025 10:06
Pankraz76
Pankraz76 commented May 13, 2025
View reviewed changes
Copy link
Contributor Author
@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

request feedback.

@Pankraz76 Pankraz76 requested a review from gnodet May 13, 2025 10:09
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 0d2a242 to 19f4281 Compare May 13, 2025 10:16
gnodet
gnodet requested changes May 13, 2025
View reviewed changes
Copy link
Contributor
@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

Definite no: 3 classes to implement 2 methods is a no go for me.

@Pankraz76 Pankraz76 changed the title Cure feature envy of DefaultPluginXmlFactory Cure feature envy of DefaultPluginXmlFactory leveraging OOP May 13, 2025
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 19f4281 to cd34ecf Compare May 13, 2025 12:57
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch 3 times, most recently from 09692d4 to 8f0ee61 Compare May 13, 2025 13:31
@Pankraz76 Pankraz76 mentioned this pull request May 13, 2025
Merged
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 8f0ee61 to 9bfb35c Compare May 13, 2025 15:52
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from f16df2e to 8410e6b Compare May 14, 2025 09:41
@Pankraz76 Pankraz76 marked this pull request as ready for review May 14, 2025 09:43
@Pankraz76 Pankraz76 changed the title Give SPOT for DefaultPluginXmlFactory leveraging OOP Give SPOT to DefaultPluginXmlFactory leveraging OOP May 15, 2025
@Pankraz76 Pankraz76 changed the title Give SPOT to DefaultPluginXmlFactory leveraging OOP Give SPOT to DefaultPluginXmlFactory cure feature envy leveraging OOP May 15, 2025
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 8410e6b to 3d0d18b Compare May 15, 2025 07:15
@Pankraz76 Pankraz76 changed the title Give SPOT to DefaultPluginXmlFactory cure feature envy leveraging OOP Give SPOT to DefaultPluginXmlFactory cure feature envy leverage OOP May 15, 2025
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 3d0d18b to 33830ea Compare May 15, 2025 07:32
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from c09f7ee to 6333f01 Compare May 15, 2025 15:26
Pankraz76
Pankraz76 commented May 15, 2025
View reviewed changes
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java Outdated
@@ -84,7 +84,7 @@ private Model doRead(XmlReaderRequest request) throws XmlReaderException {
Reader reader = request.getReader();
InputStream inputStream = request.getInputStream();
if (path == null && url == null && reader == null && inputStream == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use oop.

@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 6333f01 to 7cf25e2 Compare May 15, 2025 15:28
@Pankraz76 Pankraz76 mentioned this pull request May 15, 2025
Open
@Pankraz76 Pankraz76 requested a review from elharo May 15, 2025 15:39
Pankraz76
Pankraz76 commented May 15, 2025
View reviewed changes
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java
}

@Override
public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
Copy link
Contributor Author
@Pankraz76 Pankraz76 May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Having a factory not creating anything, seems kind of goofy:

image

This seems more service, then factory layer.

Pankraz76
Pankraz76 commented May 15, 2025
View reviewed changes
Copy link
Contributor Author
@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

kindly request critic.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java
}

@Override
public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
public void request(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {

might have generic request name propagate overload, as type seems enough to determine.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;

@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {

@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
public PluginDescriptor request(@Nonnull XmlReaderRequest request) throws XmlReaderException {

might have generic request name propagate overload, as type seems enough to determine.

elharo
elharo reviewed May 16, 2025
View reviewed changes
api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlReaderRequest.java Outdated
@@ -78,6 +78,12 @@ interface Transformer {
String transform(String source, String fieldName);
}

default void assertReadable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

verifyReadable as assert typically means something a little different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

consider generic throwIfIncomplete for both, as its kind of same use case.

@Pankraz76
Copy link
Contributor Author
Pankraz76 commented May 16, 2025 via email

@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from 7cf25e2 to d36ef1d Compare May 16, 2025 21:42
@Pankraz76 Pankraz76 changed the title Give SPOT to DefaultPluginXmlFactory cure feature envy leverage OOP extract common functionality of DefaultPluginXmlFactory May 16, 2025
@Pankraz76 Pankraz76 force-pushed the fix-write-test-cleanup branch from d36ef1d to a1d7bb0 Compare May 16, 2025 21:43
@Pankraz76 Pankraz76 requested a review from elharo May 16, 2025 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnodet gnodet Awaiting requested review from gnodet

@elharo elharo Awaiting requested review from elharo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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