Mailing-List: contact commits-help@beam.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@beam.incubator.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: lcwik@apache.org
To: commits@beam.incubator.apache.org
Date: Wed, 14 Dec 2016 06:48:03 -0000
Message-Id: <b85b118e2a7c4be7888ed9b8816a18d0@git.apache.org>
In-Reply-To: <e9b47d30c5154a53921fea55c25f70d4@git.apache.org>
References: <e9b47d30c5154a53921fea55c25f70d4@git.apache.org>
Subject: [2/3] incubator-beam git commit: Better comments and cleanup
archived-at: Wed, 14 Dec 2016 06:49:56 -0000

Better comments and cleanup


Project: http://git-wip-us.apache.org/repos/asf/incubator-beam/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-beam/commit/a0a1fea3
Tree: http://git-wip-us.apache.org/repos/asf/incubator-beam/tree/a0a1fea3
Diff: http://git-wip-us.apache.org/repos/asf/incubator-beam/diff/a0a1fea3

Branch: refs/heads/master
Commit: a0a1fea366849dec8c5dba868a71a1ccdce902eb
Parents: ae1d2a3
Author: Vikas Kedigehalli <vikasrk@google.com>
Authored: Tue Dec 13 14:16:16 2016 -0800
Committer: Luke Cwik <lcwik@google.com>
Committed: Tue Dec 13 22:37:23 2016 -0800

----------------------------------------------------------------------
 .../sdk/options/PipelineOptionsFactory.java     | 50 ++++++++++----------
 .../sdk/options/PipelineOptionsFactoryTest.java |  1 -
 2 files changed, 26 insertions(+), 25 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/a0a1fea3/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
----------------------------------------------------------------------
diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
index 2d013fd..42e1092 100644
--- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
+++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java
@@ -1652,9 +1652,11 @@ public class PipelineOptionsFactory {
     return convertedOptions;
   }
 
+
   /**
-   * Returns true if the given type is a SIMPLE_TYPES, enum or any of these types in a
-   * parameterized ValueProvider.
+   * Returns true if the given type is one of {@code SIMPLE_TYPES} or an enum, or if the given type
+   * is a {@link ValueProvider ValueProvider&lt;T&gt;} and {@code T} is one of {@code SIMPLE_TYPES}
+   * or an enum.
    */
   private static boolean isSimpleType(Class<?> type, JavaType genericType) {
     Class<?> unwrappedType = type.equals(ValueProvider.class)
@@ -1663,23 +1665,23 @@ public class PipelineOptionsFactory {
   }
 
   /**
-   * Returns true if the given type is a any Array or Collection of SIMPLE_TYPES or enum, or
-   * any of these types in a parameterized ValueProvider.
+   * Returns true if the given type is an array or {@link Collection} of {@code SIMPLE_TYPES} or
+   * enums, or if the given type is a {@link ValueProvider ValueProvider&lt;T&gt;} and {@code T} is
+   * an array or {@link Collection} of {@code SIMPLE_TYPES} or enums.
    */
   private static boolean isCollectionOrArrayOfAllowedTypes(Class<?> type, JavaType genericType) {
-    Class<?> containerType = type.equals(ValueProvider.class)
-        ? genericType.containedType(0).getRawClass() : type;
+    JavaType containerType = type.equals(ValueProvider.class)
+        ? genericType.containedType(0) : genericType;
 
     // Check if it is an array of simple types or enum.
-    if (containerType.isArray() && (SIMPLE_TYPES.contains(containerType.getComponentType())
-        || containerType.getComponentType().isEnum())) {
+    if (containerType.getRawClass().isArray()
+        && (SIMPLE_TYPES.contains(containerType.getRawClass().getComponentType())
+            || containerType.getRawClass().getComponentType().isEnum())) {
         return true;
     }
     // Check if it is Collection of simple types or enum.
-    if (Collection.class.isAssignableFrom(containerType)) {
-      JavaType innerType = type.equals(ValueProvider.class)
-          ? genericType.containedType(0).containedType(0)
-          : genericType.containedType(0);
+    if (Collection.class.isAssignableFrom(containerType.getRawClass())) {
+      JavaType innerType = containerType.containedType(0);
       // Note that raw types are allowed, hence the null check.
       if (innerType == null || SIMPLE_TYPES.contains(innerType.getRawClass())
           || innerType.getRawClass().isEnum()) {
@@ -1692,8 +1694,10 @@ public class PipelineOptionsFactory {
   /**
    * Ensures that empty string value is allowed for a given type.
    *
-   * <p>Empty strings are only allowed for String, String Array, Collection of Strings or any of
-   * these types in a parameterized ValueProvider.
+   * <p>Empty strings are only allowed for {@link String}, {@link String String[]},
+   * {@link Collection Collection&lt;String&gt;}, or {@link ValueProvider ValueProvider&lt;T&gt;}
+   * and {@code T} is of type {@link String}, {@link String String[]},
+   * {@link Collection Collection&lt;String&gt;}.
    *
    * @param type class object for the type under check.
    * @param genericType complete type information for the type under check.
@@ -1701,16 +1705,14 @@ public class PipelineOptionsFactory {
    */
   private static void checkEmptyStringAllowed(Class<?> type, JavaType genericType,
       String genericTypeName) {
-    Class<?> unwrappedType = type.equals(ValueProvider.class)
-        ? genericType.containedType(0).getRawClass() : type;
-
-    Class<?> containedType = unwrappedType;
-    if (unwrappedType.isArray()) {
-      containedType = unwrappedType.getComponentType();
-    } else if (Collection.class.isAssignableFrom(unwrappedType)) {
-      JavaType innerType = type.equals(ValueProvider.class)
-          ? genericType.containedType(0).containedType(0)
-          : genericType.containedType(0);
+    JavaType unwrappedType = type.equals(ValueProvider.class)
+        ? genericType.containedType(0) : genericType;
+
+    Class<?> containedType = unwrappedType.getRawClass();
+    if (unwrappedType.getRawClass().isArray()) {
+      containedType = unwrappedType.getRawClass().getComponentType();
+    } else if (Collection.class.isAssignableFrom(unwrappedType.getRawClass())) {
+      JavaType innerType = unwrappedType.containedType(0);
       // Note that raw types are allowed, hence the null check.
       containedType = innerType == null ? String.class : innerType.getRawClass();
     }

http://git-wip-us.apache.org/repos/asf/incubator-beam/blob/a0a1fea3/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java
----------------------------------------------------------------------
diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java
index 525b937..d73cad4 100644
--- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java
+++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java
@@ -833,7 +833,6 @@ public class PipelineOptionsFactoryTest {
     assertEquals(TestEnum.Value, options.getEnumValue().get());
   }
 
-
   @Test
   public void testStringValueProvider() {
     String[] args = new String[] {"--stringValue=beam"};