[GitHub] [maven] elharo commented on a diff in pull request #1194: [MNG-6401] Support interpolation of the proxy port in settings.xml

[via GitHub]

elharo commented on code in PR #1194:
URL: https://github.com/apache/maven/pull/1194#discussion_r1251925120


##########
api/maven-api-settings/src/main/mdo/settings.mdo:
##########
@@ -535,6 +539,62 @@
           <type>String</type>
         </field>
       </fields>
+      <codeSegments>
+        <codeSegment>
+          <version>1.0.0/1.3.0</version>
+          <code>
+            <![CDATA[
+    public boolean isActive() {
+        return (getActiveString() != null) ? 
Boolean.parseBoolean(getActiveString()) : true;
+    }
+
+    public void setActive(boolean active) {
+        setActiveString(String.valueOf(active));
+    }
+
+    public int getPort() {
+        return (getPortString() != null) ? Integer.parseInt(getPortString()) : 
8080;
+    }
+
+    public void setPort(int port) {
+        setPortString(String.valueOf(port));
+    }
+            ]]>
+          </code>
+        </codeSegment>
+        <codeSegment>
+          <version>2.0.0+</version>
+          <code>
+            <![CDATA[
+    /**
+     * Indicates if this proxy is active or not.
+     * To allow interpolation of this field, this method lazily parses
+     * the {@link #getActiveString()} value as a boolean and defaults to 
{@code true}
+     * if not set.
+     *
+     * @return a boolean indicating whether this proxy is active or not
+     */
+    public boolean isActive() {
+        return (getActiveString() != null) ? 
Boolean.parseBoolean(getActiveString()) : true;
+    }
+
+    /**
+     * Returns the port to use for this proxy.
+     * To allow interpolation of this field, this method lazily parses
+     * the {@link #getPortString()} value as an integer and defaults to {@code 
8080}
+     * if not set.
+     *
+     * @return an integer indicating the port to use for this proxy
+     * @throws NumberFormatException if the port string is not a valid integer

Review Comment:
   NumberFormatException is an IllegalArgumentException. It is very weird for a 
method with no arguments to throw an IllegalArgumentException and very 
unexpected for a getter to throw any exception at all.
   
   Where/when does interpolation happen? Can you throw the exception at that 
point and store an int for later use here?



##########
api/maven-api-settings/src/main/mdo/settings.mdo:
##########
@@ -459,17 +459,19 @@
         The <code>&lt;proxy&gt;</code> element contains informations required 
to a proxy settings.
         ]]></description>
       <fields>
-        <field>
-          <name>active</name>
+        <field xml.tagName="active">
+          <name>activeString</name>
           <version>1.0.0+</version>
           <required>false</required>
           <defaultValue>true</defaultValue>
           <description>
             <![CDATA[

Review Comment:
   It's syntax sugar, unnecessary here though and just excess code. It can be 
fixed one by one, or not, as you like. There's no need to change everything at 
once.



##########
api/maven-api-settings/src/main/mdo/settings.mdo:
##########
@@ -535,6 +539,62 @@
           <type>String</type>
         </field>
       </fields>
+      <codeSegments>
+        <codeSegment>
+          <version>1.0.0/1.3.0</version>
+          <code>
+            <![CDATA[
+    public boolean isActive() {
+        return (getActiveString() != null) ? 
Boolean.parseBoolean(getActiveString()) : true;
+    }
+
+    public void setActive(boolean active) {
+        setActiveString(String.valueOf(active));
+    }
+
+    public int getPort() {
+        return (getPortString() != null) ? Integer.parseInt(getPortString()) : 
8080;
+    }
+
+    public void setPort(int port) {
+        setPortString(String.valueOf(port));

Review Comment:
   Interesting. Why don't we? In general, the ealrier one can detect an error 
such as a negative port the better.



-- 
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-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org