You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "Pankraz76 (via GitHub)" <gi...@apache.org> on 2025/05/12 11:44:42 UTC
[PR] Resolve `feature envy` in `DefaultPluginXmlFactory` [maven]
Pankraz76 opened a new pull request, #2326:
URL: https://github.com/apache/maven/pull/2326
Resolve `feature envy` in `DefaultPluginXmlFactory`
- https://github.com/apache/maven/pull/2312
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086517997
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
Review Comment:
But that makes no sense here. There's no creation of `read` objects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2087160968
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +36,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ private static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
Review Comment:
> Will test
unable to on this lvl. need higher test for this. Powermock is outdated considering not an option.
How to test it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086821863
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlWriterRequest.java:
##########
@@ -28,7 +28,7 @@
import org.apache.maven.api.annotations.Nullable;
/**
- * An XML writer request.
+ * An XML writer
Review Comment:
mistake of s&r, sorry. revert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Give ` SPOT` to `DefaultPluginXmlFactory` cure `feature envy` leverage `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2093703834
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlReaderRequest.java:
##########
@@ -78,6 +78,12 @@ interface Transformer {
String transform(String source, String fieldName);
}
+ default void assertReadable() {
Review Comment:
consider generic `throwIfIncomplete` for both, as its kind of same use case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve `feature envy` in `DefaultPluginXmlFactory` [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2326:
URL: https://github.com/apache/maven/pull/2326#issuecomment-2873714730
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086817789
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlWriterRequest.java:
##########
@@ -28,7 +28,7 @@
import org.apache.maven.api.annotations.Nullable;
/**
- * An XML writer request.
+ * An XML writer
Review Comment:
Why ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086444210
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,67 +37,25 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ public static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
Review Comment:
for silliness like this we need proper tooling. Spot is obviously not enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086763458
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +37,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ var req = request.validateReadability();
Review Comment:
good example for OOP.
https://en.wikipedia.org/wiki/Object-oriented_programming
Moving domain specific logic, to its origin where its dedicated to and designated from; making context clear; giving second purpose to be reused.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086803705
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultPluginXmlFactoryTest.java:
##########
@@ -0,0 +1,297 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.impl;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.StringReader;
+import java.io.StringWriter;
+import java.io.Writer;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+
+import com.ctc.wstx.exc.WstxEOFException;
+import org.apache.maven.api.plugin.descriptor.PluginDescriptor;
+import org.apache.maven.api.services.xml.ModelXmlFactory;
+import org.apache.maven.api.services.xml.XmlReaderException;
+import org.apache.maven.api.services.xml.XmlReaderRequest;
+import org.apache.maven.api.services.xml.XmlWriterException;
+import org.apache.maven.api.services.xml.XmlWriterRequest;
+import org.apache.maven.impl.model.DefaultModelProcessor;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static java.util.UUID.randomUUID;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.when;
+
+class DefaultPluginXmlFactoryReadWriteTest {
+
+ private static final String NAME = "sample-plugin-" + randomUUID();
+ private static final String SAMPLE_PLUGIN_XML =
+ """
+ <?xml version="1.0" encoding="UTF-8"?>
+ <plugin>
+ <name>%s</name>
+ <groupId>org.example</groupId>
+ <artifactId>sample-plugin</artifactId>
+ <version>1.0.0</version>
+ </plugin>
+ """
+ .formatted(NAME);
+
+ private final DefaultPluginXmlFactory defaultPluginXmlFactory = new DefaultPluginXmlFactory();
+
+ @TempDir
+ Path tempDir;
+
+ @Test
+ void readFromInputStreamParsesPluginDescriptorCorrectly() {
+ PluginDescriptor descriptor = defaultPluginXmlFactory.read(XmlReaderRequest.builder()
+ .inputStream(new ByteArrayInputStream(SAMPLE_PLUGIN_XML.getBytes()))
+ .build());
+ assertThat(descriptor.getName()).isEqualTo(NAME);
+ assertThat(descriptor.getGroupId()).isEqualTo("org.example");
+ assertThat(descriptor.getArtifactId()).isEqualTo("sample-plugin");
+ assertThat(descriptor.getVersion()).isEqualTo("1.0.0");
+ }
+
+ @Test
+ void parsePlugin() {
+ assertThat(defaultPluginXmlFactory
+ .read(XmlReaderRequest.builder()
+ .reader(new StringReader(SAMPLE_PLUGIN_XML))
+ .build())
+ .getName())
+ .isEqualTo(NAME);
+ }
+
+ @Test
+ @Disabled
+ void readFromPathParsesPluginDescriptorCorrectly() throws Exception {
+ Path xmlFile = tempDir.resolve("plugin.xml");
+ Files.write(xmlFile, SAMPLE_PLUGIN_XML.getBytes());
+ assertThat(defaultPluginXmlFactory
+ .read(XmlReaderRequest.builder().path(xmlFile).build())
+ .getName())
+ .isEqualTo(NAME);
+ }
+
+ @Test
+ void readWithNoInputThrowsIllegalArgumentException() {
+ assertThatExceptionOfType(IllegalArgumentException.class)
+ .isThrownBy(() ->
+ defaultPluginXmlFactory.read(XmlReaderRequest.builder().build()));
+ }
+
+ @Test
+ void writeToWriterGeneratesValidXml() {
+ StringWriter writer = new StringWriter();
+ defaultPluginXmlFactory.write(XmlWriterRequest.<PluginDescriptor>builder()
+ .writer(writer)
+ .content(PluginDescriptor.newBuilder()
+ .name(NAME)
+ .groupId("org.example")
+ .artifactId("sample-plugin")
+ .version("1.0.0")
+ .build())
+ .build());
+ String output = writer.toString();
+ assertThat(output).contains("<name>" + NAME + "</name>");
+ assertThat(output).contains("<groupId>org.example</groupId>");
+ }
+
+ @Test
+ void writeToOutputStreamGeneratesValidXml() {
+ ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+ defaultPluginXmlFactory.write(XmlWriterRequest.<PluginDescriptor>builder()
+ .outputStream(outputStream)
+ .content(PluginDescriptor.newBuilder().name(NAME).build())
+ .build());
+ assertThat(outputStream.toString()).contains("<name>" + NAME + "</name>");
+ }
+
+ @Test
+ void writeToPathGeneratesValidXmlFile() throws Exception {
+ Path xmlFile = tempDir.resolve("output-plugin.xml");
+ defaultPluginXmlFactory.write(XmlWriterRequest.<PluginDescriptor>builder()
+ .path(xmlFile)
+ .content(PluginDescriptor.newBuilder().name(NAME).build())
+ .build());
+ assertThat(Files.readString(xmlFile)).contains("<name>" + NAME + "</name>");
+ }
+
+ @Test
+ void fromXmlStringParsesValidXml() {
+ PluginDescriptor descriptor = defaultPluginXmlFactory.fromXmlString(SAMPLE_PLUGIN_XML);
+ assertThat(descriptor.getName()).isEqualTo(NAME);
+ assertThat(descriptor.getGroupId()).isEqualTo("org.example");
+ assertThat(descriptor.getArtifactId()).isEqualTo("sample-plugin");
+ assertThat(descriptor.getVersion()).isEqualTo("1.0.0");
+ }
+
+ @Test
+ void toXmlStringGeneratesValidXml() {
+ String xml = defaultPluginXmlFactory.toXmlString(PluginDescriptor.newBuilder()
+ .name(NAME)
+ .groupId("org.example")
+ .artifactId("sample-plugin")
+ .version("1.0.0")
+ .build());
+ assertThat(xml).contains("<name>" + NAME + "</name>");
+ assertThat(xml).contains("<groupId>org.example</groupId>");
+ assertThat(xml).contains("<artifactId>sample-plugin</artifactId>");
+ assertThat(xml).contains("<version>1.0.0</version>");
+ }
+
+ @Test
+ void staticFromXmlParsesValidXml() {
+ PluginDescriptor descriptor = DefaultPluginXmlFactory.fromXml(SAMPLE_PLUGIN_XML);
+ assertThat(descriptor.getName()).isEqualTo(NAME);
+ assertThat(descriptor.getGroupId()).isEqualTo("org.example");
+ assertThat(descriptor.getArtifactId()).isEqualTo("sample-plugin");
+ assertThat(descriptor.getVersion()).isEqualTo("1.0.0");
+ }
+
+ @Test
+ void staticToXmlGeneratesValidXml() {
+ String xml = DefaultPluginXmlFactory.toXml(PluginDescriptor.newBuilder()
+ .name(NAME)
+ .groupId("org.example")
+ .artifactId("sample-plugin")
+ .version("1.0.0")
+ .build());
+ assertThat(xml).contains("<name>" + NAME + "</name>");
+ assertThat(xml).contains("<name>" + NAME + "</name>");
+ assertThat(xml).contains("<groupId>org.example</groupId>");
+ assertThat(xml).contains("<artifactId>sample-plugin</artifactId>");
+ assertThat(xml).contains("<version>1.0.0</version>");
+ }
+
+ @Test
+ void writeWithFailingWriterThrowsXmlWriterException() {
+ String unableToWritePlugin = "Unable to write plugin" + randomUUID();
+ String ioEx = "ioEx" + randomUUID();
+ XmlWriterException exception = assertThatExceptionOfType(XmlWriterException.class)
+ .isThrownBy(() -> defaultPluginXmlFactory.write(XmlWriterRequest.<PluginDescriptor>builder()
+ .writer(new Writer() {
+ @Override
+ public void write(char[] cbuf, int off, int len) {
+ throw new XmlWriterException(unableToWritePlugin, null, new IOException(ioEx));
+ }
+
+ @Override
+ public void flush() {}
+
+ @Override
+ public void close() {}
+ })
+ .content(PluginDescriptor.newBuilder()
+ .name("Failing Plugin")
+ .build())
+ .build()))
+ .actual();
+ assertThat(exception.getMessage()).contains(unableToWritePlugin);
+ assertThat(exception.getCause()).isInstanceOf(XmlWriterException.class);
+ assertThat(exception.getCause().getCause().getMessage()).isEqualTo(ioEx);
+ assertThat(exception.getCause().getMessage()).isEqualTo(unableToWritePlugin);
+ }
+
+ @Test
+ void writeWithNoTargetThrowsIllegalArgumentException() {
+ assertThat(assertThrows(
+ IllegalArgumentException.class,
+ () -> defaultPluginXmlFactory.write(XmlWriterRequest.<PluginDescriptor>builder()
+ .content(PluginDescriptor.newBuilder()
+ .name("No Output Plugin")
+ .build())
+ .build()))
+ .getMessage())
+ .isEqualTo("writer, outputStream or path must be non null");
+ }
+
+ @Test
+ void readMalformedXmlThrowsXmlReaderException() {
+ XmlReaderException exception = assertThatExceptionOfType(XmlReaderException.class)
+ .isThrownBy(() -> defaultPluginXmlFactory.read(XmlReaderRequest.builder()
+ .inputStream(new ByteArrayInputStream("<plugin><name>Broken Plugin".getBytes()))
+ .build()))
+ .actual();
+ assertThat(exception.getMessage()).contains("Unable to read plugin");
+ assertThat(exception.getCause()).isInstanceOf(WstxEOFException.class);
+ }
+
+ @Test
+ void locateExistingPomWithFilePathShouldReturnSameFileIfRegularFile() throws IOException {
+ Path pomFile = Files.createTempFile(tempDir, "pom", ".xml");
+ DefaultModelProcessor processor = new DefaultModelProcessor(mock(ModelXmlFactory.class), List.of());
+ assertThat(processor.locateExistingPom(pomFile)).isEqualTo(pomFile);
+ }
+
+ @Test
+ void readFromUrlParsesPluginDescriptorCorrectly() throws Exception {
+ Path xmlFile = tempDir.resolve("plugin.xml");
+ Files.write(xmlFile, SAMPLE_PLUGIN_XML.getBytes());
+ PluginDescriptor descriptor = defaultPluginXmlFactory.read(XmlReaderRequest.builder()
+ .inputStream(xmlFile.toUri().toURL().openStream())
+ .build());
+ assertThat(descriptor.getName()).isEqualTo(NAME);
+ assertThat(descriptor.getGroupId()).isEqualTo("org.example");
+ assertThat(descriptor.getArtifactId()).isEqualTo("sample-plugin");
+ assertThat(descriptor.getVersion()).isEqualTo("1.0.0");
+ }
+
+ @Test
+ void testReadWithPath() throws Exception {
+ Path tempPath = Files.createTempFile("plugin", ".xml");
+ Files.writeString(tempPath, "<plugin/>");
+ XmlReaderRequest request = spy(XmlReaderRequest.class);
Review Comment:
<img width="1900" alt="image" src="https://github.com/user-attachments/assets/863fad68-0949-40bd-bb8e-0e6a6fb73f53" />
require `spy` now, which indicates strong test, as a lot is going on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extract common functionality of DefaultPluginXmlFactory [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2094547990
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +37,63 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
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 {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ return read(request, setAddDefaultEntities(request, new PluginDescriptorStaxReader()));
+ }
+
+ @Override
+ public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
Review Comment:
Agreed, but in that case, the PR is off-topic and maybe the point is to rename the interfaces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve `feature envy` in `DefaultPluginXmlFactory` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2085375181
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
- try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
- xml.setAddDefaultEntities(request.isAddDefaultEntities());
- if (inputStream != null) {
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
- nonNull(request, "request");
- Path path = request.getPath();
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ReadRequest.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.impl;
+
+import javax.xml.stream.XMLStreamException;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.apache.maven.api.plugin.descriptor.PluginDescriptor;
+import org.apache.maven.api.services.xml.XmlReaderException;
+import org.apache.maven.api.services.xml.XmlReaderRequest;
+import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
+
+import static org.apache.maven.impl.StaxLocation.getLocation;
+import static org.apache.maven.impl.StaxLocation.getMessage;
+
+record ReadRequest(
+ Path path, URL url, Reader reader, InputStream inputStream, boolean addDefaultEntities, boolean strict) {
+
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
+ ReadRequest(XmlReaderRequest request) {
+ this(
+ request.getPath(),
+ request.getURL(),
+ request.getReader(),
+ request.getInputStream(),
+ request.isAddDefaultEntities(),
+ request.isStrict());
+ if (inputStream == null && reader == null && path == null && url == null) {
+ throw new IllegalArgumentException("writer, outputStream or path must be non null");
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
Review Comment:
```suggestion
public PluginDescriptor createReadPlugin(@Nonnull XmlReaderRequest request) throws XmlReaderException {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ReadRequest.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.impl;
+
+import javax.xml.stream.XMLStreamException;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.apache.maven.api.plugin.descriptor.PluginDescriptor;
+import org.apache.maven.api.services.xml.XmlReaderException;
+import org.apache.maven.api.services.xml.XmlReaderRequest;
+import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
+
+import static org.apache.maven.impl.StaxLocation.getLocation;
+import static org.apache.maven.impl.StaxLocation.getMessage;
+
+record ReadRequest(
+ Path path, URL url, Reader reader, InputStream inputStream, boolean addDefaultEntities, boolean strict) {
+
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
+ ReadRequest(XmlReaderRequest request) {
+ this(
+ request.getPath(),
+ request.getURL(),
+ request.getReader(),
+ request.getInputStream(),
+ request.isAddDefaultEntities(),
+ request.isStrict());
+ if (inputStream == null && reader == null && path == null && url == null) {
+ throw new IllegalArgumentException("writer, outputStream or path must be non null");
Review Comment:
yes this is actually domain logic of XmlReaderRequest or XmlReaderRequestValidator.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ReadRequest.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.impl;
+
+import javax.xml.stream.XMLStreamException;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.apache.maven.api.plugin.descriptor.PluginDescriptor;
+import org.apache.maven.api.services.xml.XmlReaderException;
+import org.apache.maven.api.services.xml.XmlReaderRequest;
+import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
+
+import static org.apache.maven.impl.StaxLocation.getLocation;
+import static org.apache.maven.impl.StaxLocation.getMessage;
+
+record ReadRequest(
+ Path path, URL url, Reader reader, InputStream inputStream, boolean addDefaultEntities, boolean strict) {
+
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
+ ReadRequest(XmlReaderRequest request) {
+ this(
+ request.getPath(),
+ request.getURL(),
+ request.getReader(),
+ request.getInputStream(),
+ request.isAddDefaultEntities(),
+ request.isStrict());
+ if (inputStream == null && reader == null && path == null && url == null) {
+ throw new IllegalArgumentException("writer, outputStream or path must be non null");
+ }
+ READER.setAddDefaultEntities(addDefaultEntities);
+ }
+
+ PluginDescriptor read() throws XmlReaderException {
+ try {
+ if (inputStream != null) {
+ return ReadRequest.READER.read(inputStream, strict);
+ } else if (reader != null) {
+ return ReadRequest.READER.read(reader, strict);
+ } else if (path != null) {
+ try (InputStream is = Files.newInputStream(path)) {
+ return ReadRequest.READER.read(is, strict);
+ }
+ }
+ try (InputStream is = url.openStream()) {
+ return ReadRequest.READER.read(is, strict);
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
- try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
- xml.setAddDefaultEntities(request.isAddDefaultEntities());
- if (inputStream != null) {
- return xml.read(inputStream, request.isStrict());
- } else if (reader != null) {
- return xml.read(reader, request.isStrict());
- } else if (path != null) {
- try (InputStream is = Files.newInputStream(path)) {
- return xml.read(is, request.isStrict());
- }
- } else {
- try (InputStream is = url.openStream()) {
- return xml.read(is, request.isStrict());
- }
- }
- } catch (Exception e) {
- throw new XmlReaderException("Unable to read plugin: " + getMessage(e), getLocation(e), e);
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
- try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ReadRequest.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.impl;
+
+import javax.xml.stream.XMLStreamException;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.apache.maven.api.plugin.descriptor.PluginDescriptor;
+import org.apache.maven.api.services.xml.XmlReaderException;
+import org.apache.maven.api.services.xml.XmlReaderRequest;
+import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
+
+import static org.apache.maven.impl.StaxLocation.getLocation;
+import static org.apache.maven.impl.StaxLocation.getMessage;
+
+record ReadRequest(
+ Path path, URL url, Reader reader, InputStream inputStream, boolean addDefaultEntities, boolean strict) {
+
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
+ ReadRequest(XmlReaderRequest request) {
+ this(
+ request.getPath(),
+ request.getURL(),
+ request.getReader(),
+ request.getInputStream(),
+ request.isAddDefaultEntities(),
+ request.isStrict());
+ if (inputStream == null && reader == null && path == null && url == null) {
+ throw new IllegalArgumentException("writer, outputStream or path must be non null");
+ }
+ READER.setAddDefaultEntities(addDefaultEntities);
+ }
+
+ PluginDescriptor read() throws XmlReaderException {
+ try {
+ if (inputStream != null) {
+ return ReadRequest.READER.read(inputStream, strict);
+ } else if (reader != null) {
+ return ReadRequest.READER.read(reader, strict);
+ } else if (path != null) {
+ try (InputStream is = Files.newInputStream(path)) {
+ return ReadRequest.READER.read(is, strict);
+ }
+ }
+ try (InputStream is = url.openStream()) {
+ return ReadRequest.READER.read(is, strict);
+ }
+ } catch (IOException | XMLStreamException e) {
+ throw new XmlReaderException("Unable to read plugin: " + getMessage(e), getLocation(e), e);
Review Comment:
SOC
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
Review Comment:
Then this name is kind of wrong. Why should a factory read?
Its producing goods therefore providing `createReadPlugin` or `createRead`
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-import static org.apache.maven.impl.StaxLocation.getLocation;
-import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
Review Comment:
Still factory is only creational pattern, having to delegate like everybody else following SOLID.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve `feature envy` in `DefaultPluginXmlFactory` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on PR #2326:
URL: https://github.com/apache/maven/pull/2326#issuecomment-2873893011
sacrifice/spike architectural design again, in sake to increment code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086441721
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,67 +37,25 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ public static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
- try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
- xml.setAddDefaultEntities(request.isAddDefaultEntities());
- if (inputStream != null) {
- return xml.read(inputStream, request.isStrict());
- } else if (reader != null) {
- return xml.read(reader, request.isStrict());
- } else if (path != null) {
- try (InputStream is = Files.newInputStream(path)) {
- return xml.read(is, request.isStrict());
- }
- } else {
- try (InputStream is = url.openStream()) {
- return xml.read(is, request.isStrict());
- }
- }
- } catch (Exception e) {
- throw new XmlReaderException("Unable to read plugin: " + getMessage(e), getLocation(e), e);
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ return createReadDescriptor(validate(request));
}
@Override
public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
- nonNull(request, "request");
- PluginDescriptor content = nonNull(request.getContent(), "content");
- Path path = request.getPath();
- OutputStream outputStream = request.getOutputStream();
- Writer writer = request.getWriter();
- if (writer == null && outputStream == null && path == null) {
- throw new IllegalArgumentException("writer, outputStream or path must be non null");
- }
- try {
- if (writer != null) {
- new PluginDescriptorStaxWriter().write(writer, content);
Review Comment:
factory is supposed to create; not to be the creation. Its actually an service layer and should follow delegation to embrace SOLID. Bug this is arch-design. Test fix implementation code first then think about big picture.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086820174
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlWriterRequest.java:
##########
@@ -55,6 +55,13 @@ static <T> XmlWriterRequestBuilder<T> builder() {
return new XmlWriterRequestBuilder<>();
}
+ default XmlWriterRequest<T> assertWritable() {
+ if (getWriter() == null && getOutputStream() == null && getPath() == null) {
+ throw new IllegalArgumentException("writer, outputStream or path must be non null");
+ }
+ return this;
+ }
Review Comment:
could live somewhere else. Companion or Util.
It belongs here obviously, as everything else require more code, but we can take it away.
Matter of arch-design.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extract common functionality of DefaultPluginXmlFactory [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 closed pull request #2326: extract common functionality of DefaultPluginXmlFactory
URL: https://github.com/apache/maven/pull/2326
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve `feature envy` in `DefaultPluginXmlFactory` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on PR #2326:
URL: https://github.com/apache/maven/pull/2326#issuecomment-2873836165
https://refactoring.guru/design-patterns/factory-method
Considering factory adhering to creational design pattern its supposed to `create` (what) stuff like shown in pr, not to `do` (how).
> 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 im sure its not one and only.
Grouping data and its entourage; companion into dedicated classified dimensions.
- https://en.wikipedia.org/wiki/Object-oriented_programming
- https://en.wikipedia.org/wiki/Inversion_of_control
When this factory is handling 100 products im sure they wont all gather inside on place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve `feature envy` in `DefaultPluginXmlFactory` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on PR #2326:
URL: https://github.com/apache/maven/pull/2326#issuecomment-2873857919
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086815063
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +37,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ var read = request.assertReadable();
try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
- xml.setAddDefaultEntities(request.isAddDefaultEntities());
- if (inputStream != null) {
- return xml.read(inputStream, request.isStrict());
- } else if (reader != null) {
- return xml.read(reader, request.isStrict());
- } else if (path != null) {
- try (InputStream is = Files.newInputStream(path)) {
- return xml.read(is, request.isStrict());
- }
- } else {
- try (InputStream is = url.openStream()) {
- return xml.read(is, request.isStrict());
+ if (read.getInputStream() != null) {
+ return READER.read(read.getInputStream(), read.isStrict());
+ } else if (read.getReader() != null) {
+ return READER.read(read.getReader(), read.isStrict());
+ } else if (read.getPath() != null) {
+ try (InputStream is = Files.newInputStream(read.getPath())) {
+ return READER.read(is, read.isStrict());
}
}
- } catch (Exception e) {
+ try (InputStream is = read.getURL().openStream()) {
+ return READER.read(is, read.isStrict());
+ }
+ } catch (IOException | XMLStreamException e) {
throw new XmlReaderException("Unable to read plugin: " + getMessage(e), getLocation(e), e);
}
}
@Override
public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
- nonNull(request, "request");
- PluginDescriptor content = nonNull(request.getContent(), "content");
- Path path = request.getPath();
- OutputStream outputStream = request.getOutputStream();
- Writer writer = request.getWriter();
- if (writer == null && outputStream == null && path == null) {
- throw new IllegalArgumentException("writer, outputStream or path must be non null");
- }
Review Comment:
why must factory have such `random/domain` details. All we need is some valid request. No need to take hostage on this concern handling is here. Lets trade `feature envy` for `delegation` and `segregation`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086821863
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/xml/XmlWriterRequest.java:
##########
@@ -28,7 +28,7 @@
import org.apache.maven.api.annotations.Nullable;
/**
- * An XML writer request.
+ * An XML writer
Review Comment:
mistake revert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086842539
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +36,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ private static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ var read = request.assertReadable();
try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
- xml.setAddDefaultEntities(request.isAddDefaultEntities());
- if (inputStream != null) {
- return xml.read(inputStream, request.isStrict());
- } else if (reader != null) {
- return xml.read(reader, request.isStrict());
- } else if (path != null) {
- try (InputStream is = Files.newInputStream(path)) {
- return xml.read(is, request.isStrict());
- }
- } else {
- try (InputStream is = url.openStream()) {
- return xml.read(is, request.isStrict());
+ if (read.getInputStream() != null) {
+ return READER.read(read.getInputStream(), read.isStrict());
+ } else if (read.getReader() != null) {
+ return READER.read(read.getReader(), read.isStrict());
+ } else if (read.getPath() != null) {
+ try (InputStream is = Files.newInputStream(read.getPath())) {
+ return READER.read(is, read.isStrict());
}
}
- } catch (Exception e) {
+ try (var is = read.getURL().openStream()) {
Review Comment:
```suggestion
try (InputStream is = read.getURL().openStream()) {
```
Many languages use `val` and `var` as type it not first priority. less seems more to me.
- https://github.com/apache/maven/pull/2236
- https://github.com/apache/maven/pull/2236/files#r2029871372
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086815063
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +37,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ var read = request.assertReadable();
try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
- xml.setAddDefaultEntities(request.isAddDefaultEntities());
- if (inputStream != null) {
- return xml.read(inputStream, request.isStrict());
- } else if (reader != null) {
- return xml.read(reader, request.isStrict());
- } else if (path != null) {
- try (InputStream is = Files.newInputStream(path)) {
- return xml.read(is, request.isStrict());
- }
- } else {
- try (InputStream is = url.openStream()) {
- return xml.read(is, request.isStrict());
+ if (read.getInputStream() != null) {
+ return READER.read(read.getInputStream(), read.isStrict());
+ } else if (read.getReader() != null) {
+ return READER.read(read.getReader(), read.isStrict());
+ } else if (read.getPath() != null) {
+ try (InputStream is = Files.newInputStream(read.getPath())) {
+ return READER.read(is, read.isStrict());
}
}
- } catch (Exception e) {
+ try (InputStream is = read.getURL().openStream()) {
+ return READER.read(is, read.isStrict());
+ }
+ } catch (IOException | XMLStreamException e) {
throw new XmlReaderException("Unable to read plugin: " + getMessage(e), getLocation(e), e);
}
}
@Override
public void write(XmlWriterRequest<PluginDescriptor> request) throws XmlWriterException {
- nonNull(request, "request");
- PluginDescriptor content = nonNull(request.getContent(), "content");
- Path path = request.getPath();
- OutputStream outputStream = request.getOutputStream();
- Writer writer = request.getWriter();
- if (writer == null && outputStream == null && path == null) {
- throw new IllegalArgumentException("writer, outputStream or path must be non null");
- }
Review Comment:
why must factory have such details. All we need is some valid request. No need to take hostage on this concern handling is here. Lets trade `feature envy` for `delegation` and `segregation`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086825830
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -35,70 +27,19 @@
import org.apache.maven.api.services.xml.XmlReaderRequest;
import org.apache.maven.api.services.xml.XmlWriterException;
import org.apache.maven.api.services.xml.XmlWriterRequest;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
-import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-
-import static org.apache.maven.impl.ImplUtils.nonNull;
-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 {
Review Comment:
yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086957507
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +36,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ private static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
Review Comment:
This cannot be threadsafe, and that's one reason why they are not singletons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2087087800
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +36,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ private static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
Review Comment:
yes wanted to challenge this. Will test. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086763458
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +37,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ var req = request.validateReadability();
Review Comment:
Moving domain specific logic, to its origin where its dedicated to and designated from; making context clear; giving second purpose to be reused.
imho a good example for OOP: https://en.wikipedia.org/wiki/Object-oriented_programming
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086763458
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +37,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ var req = request.validateReadability();
Review Comment:
perfect example for OOP
https://en.wikipedia.org/wiki/Object-oriented_programming
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Cure `feature envy` of `DefaultPluginXmlFactory` leveraging `OOP` [maven]
Posted by "Pankraz76 (via GitHub)" <gi...@apache.org>.
Pankraz76 commented on code in PR #2326:
URL: https://github.com/apache/maven/pull/2326#discussion_r2086842539
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPluginXmlFactory.java:
##########
@@ -38,65 +36,54 @@
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxReader;
import org.apache.maven.plugin.descriptor.io.PluginDescriptorStaxWriter;
-import static org.apache.maven.impl.ImplUtils.nonNull;
import static org.apache.maven.impl.StaxLocation.getLocation;
import static org.apache.maven.impl.StaxLocation.getMessage;
@Named
@Singleton
public class DefaultPluginXmlFactory implements PluginXmlFactory {
+
+ private static final PluginDescriptorStaxWriter WRITER = new PluginDescriptorStaxWriter();
+ private static final PluginDescriptorStaxReader READER = new PluginDescriptorStaxReader();
+
@Override
public PluginDescriptor read(@Nonnull XmlReaderRequest request) throws XmlReaderException {
- nonNull(request, "request");
- Path path = request.getPath();
- URL url = request.getURL();
- Reader reader = request.getReader();
- InputStream inputStream = request.getInputStream();
- if (path == null && url == null && reader == null && inputStream == null) {
- throw new IllegalArgumentException("path, url, reader or inputStream must be non null");
- }
+ READER.setAddDefaultEntities(request.isAddDefaultEntities());
+ var read = request.assertReadable();
try {
- PluginDescriptorStaxReader xml = new PluginDescriptorStaxReader();
- xml.setAddDefaultEntities(request.isAddDefaultEntities());
- if (inputStream != null) {
- return xml.read(inputStream, request.isStrict());
- } else if (reader != null) {
- return xml.read(reader, request.isStrict());
- } else if (path != null) {
- try (InputStream is = Files.newInputStream(path)) {
- return xml.read(is, request.isStrict());
- }
- } else {
- try (InputStream is = url.openStream()) {
- return xml.read(is, request.isStrict());
+ if (read.getInputStream() != null) {
+ return READER.read(read.getInputStream(), read.isStrict());
+ } else if (read.getReader() != null) {
+ return READER.read(read.getReader(), read.isStrict());
+ } else if (read.getPath() != null) {
+ try (InputStream is = Files.newInputStream(read.getPath())) {
+ return READER.read(is, read.isStrict());
}
}
- } catch (Exception e) {
+ try (var is = read.getURL().openStream()) {
Review Comment:
```suggestion
try (InputStream is = read.getURL().openStream()) {
```
Many languages use `val` and `var` as type it not first priority. less seems more to me.
- https://github.com/apache/maven/pull/2236
- https://github.com/apache/maven/pull/2236/files#r2029871372
- https://projectlombok.org/features/val
- https://kotlinlang.org/docs/basic-syntax.html#variables
- https://docs.scala-lang.org/overviews/scala-book/two-types-variables.html#the-difference-between-val-and-var
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org