You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@seatunnel.apache.org by GitBox <gi...@apache.org> on 2022/02/09 11:56:17 UTC
[GitHub] [incubator-seatunnel] simon824 opened a new pull request #1214: [hotfix] Add List type check for CheckConfigUtil
simon824 opened a new pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214
## Purpose of this pull request
Add List type check for CheckConfigUtil
## Check list
* [ ] Code changed are covered with tests, or it does not need tests for reason:
* [ ] If any new Jar binary package adding in you PR, please add License Notice according
[New License Guide](https://github.com/apache/incubator-seatunnel/blob/dev/docs/en/developement/NewLicenseGuide.md)
* [ ] If necessary, please update the documentation to describe the new feature. https://github.com/apache/incubator-seatunnel/tree/dev/docs
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] simon824 commented on pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
simon824 commented on pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#issuecomment-1034707474
cc @CalvinKirs @leo65535 ,thx.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
yx91490 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805750487
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,14 +72,14 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
- public static boolean isInvalidParam(Config config, String param) {
- boolean isInvalidParam = false;
- if (!config.hasPath(param) || config.getAnyRef(param) == null) {
- isInvalidParam = true;
+ public static boolean isValidParam(Config config, String param) {
+ boolean isValidParam = true;
+ if (!config.hasPath(param)) {
+ isValidParam = false;
} else if (config.getAnyRef(param) instanceof List) {
- isInvalidParam = ((List<?>) config.getAnyRef(param)).size() == 0;
+ isValidParam = !((List<?>) config.getAnyRef(param)).isEmpty();
}
- return isInvalidParam;
+ return isValidParam;
Review comment:
can be simplified to `return config.hasPath(param) && !((List<?>) config.getAnyRef(param)).isEmpty();`
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
yx91490 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805839780
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,14 +72,14 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
- public static boolean isInvalidParam(Config config, String param) {
- boolean isInvalidParam = false;
- if (!config.hasPath(param) || config.getAnyRef(param) == null) {
- isInvalidParam = true;
+ public static boolean isValidParam(Config config, String param) {
+ boolean isValidParam = true;
+ if (!config.hasPath(param)) {
+ isValidParam = false;
Review comment:
can `return false` directly
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
yx91490 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805131045
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
+ isInvalidParam = true;
+ } else if (config.getAnyRef(param) instanceof List) {
Review comment:
such as intList, do you have idea to check list type?
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
+ isInvalidParam = true;
+ } else if (config.getAnyRef(param) instanceof List) {
+ isInvalidParam = ((List<?>) config.getAnyRef(param)).size() == 0;
Review comment:
can be simplified to config.getAnyRefList(param).isEmpty()
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
Review comment:
It'll be more easy to understand to change to positive method name like 'isValidParam'
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
Review comment:
can be simplified, `config.hasPath()` can ensure `config.getAnyRef(param) != null`
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
yx91490 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805750487
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,14 +72,14 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
- public static boolean isInvalidParam(Config config, String param) {
- boolean isInvalidParam = false;
- if (!config.hasPath(param) || config.getAnyRef(param) == null) {
- isInvalidParam = true;
+ public static boolean isValidParam(Config config, String param) {
+ boolean isValidParam = true;
+ if (!config.hasPath(param)) {
+ isValidParam = false;
} else if (config.getAnyRef(param) instanceof List) {
- isInvalidParam = ((List<?>) config.getAnyRef(param)).size() == 0;
+ isValidParam = !((List<?>) config.getAnyRef(param)).isEmpty();
}
- return isInvalidParam;
+ return isValidParam;
Review comment:
can be simplified to `return config.hasPath(param) && !((List<?>) config.getAnyRef(param)).isEmpty();`
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] simon824 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
simon824 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805447791
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
Review comment:
what about if key exists, but value is null
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
+ isInvalidParam = true;
+ } else if (config.getAnyRef(param) instanceof List) {
Review comment:
We can pass in a type parameter.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] CalvinKirs merged pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
CalvinKirs merged pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] simon824 commented on pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
simon824 commented on pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#issuecomment-1042651331
@CalvinKirs PTAL, 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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
yx91490 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805504009
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
Review comment:
as javadoc https://lightbend.github.io/config/latest/api/index.html says, it will return false:
```
Checks whether a value is present and non-null at the given path. This differs in two ways from Map.containsKey() as implemented by [ConfigObject](https://lightbend.github.io/config/latest/api/com/typesafe/config/ConfigObject.html): it looks for a path expression, not a key; and it returns false for null values, while containsKey() returns true indicating that the object contains a null value for the key.
```
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] simon824 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
simon824 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805447791
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
Review comment:
what about if key exists, but value is null
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
+ isInvalidParam = true;
+ } else if (config.getAnyRef(param) instanceof List) {
Review comment:
We can pass in a type parameter.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] simon824 commented on a change in pull request #1214: [hotfix] Add List type check for CheckConfigUtil
Posted by GitBox <gi...@apache.org>.
simon824 commented on a change in pull request #1214:
URL: https://github.com/apache/incubator-seatunnel/pull/1214#discussion_r805528116
##########
File path: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/CheckConfigUtil.java
##########
@@ -72,6 +72,16 @@ public static CheckResult checkAtLeastOneExists(Config config, String... params)
}
}
+ public static boolean isInvalidParam(Config config, String param) {
+ boolean isInvalidParam = false;
+ if (!config.hasPath(param) || config.getAnyRef(param) == null) {
Review comment:
make sense, thx.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org