[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732528533
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
done. Seems to work fine.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
```
} catch (URISyntaxException e) {
return getWebappPath(DEFAULT_BASE_URL);
}
```
Will not compile. It will throw an exception in the catch block. Which will
just not compile. I also don't see why you need to split this into 2 methods.
> your current code will return https://localhost:5443/openmeetings
No it won't. It says ".getPath()" so it won't. DEFAULT_BASE_URL is actually
the same it will use by default.
There is no different between our alternatives. Except your alternative
splits it into 2 methods. For no obvious reason.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
```
} catch (URISyntaxException e) {
return getWebappPath(DEFAULT_BASE_URL);
}
```
Will not compile. It will throw an exception in the catch block. Which will
just not compile. I also don't see why you need to split this into 2 methods.
> your current code will return https://localhost:5443/openmeetings
No it won't. It says ".getPath()" so it won't. DEFAULT_BASE_URL is actually
the same it will test with by default.
There is no different between our alternatives. Except your alternative
splits it into 2 methods. For no obvious reason.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
```
} catch (URISyntaxException e) {
return getWebappPath(DEFAULT_BASE_URL);
}
```
Will not compile. It will throw an exception in the catch block. Which will
just not compile. I also don't see why you need to split this into 2 methods.
> your current code will return https://localhost:5443/openmeetings
No it won't. It says ".getPath()" so it won't. I was testing it.
There is no different between our alternatives. Except your alternative
splits it into 2 methods. For no obvious reason.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732447851
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
```
} catch (URISyntaxException e) {
return getWebappPath(DEFAULT_BASE_URL);
}
```
Will not compile. It will throw an exception in the catch block. Which will
just not compile. I also don't see why you need to split this into 2 methods.
> your current code will return https://localhost:5443/openmeetings
No it won't. It says ".getPath()" so it won't. I was testing it.
There is no different between our alternatives. Except your alternative
splits it into 2 methods. For no obvious reason.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732403113
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
> Sure, let it log :) I believe WARN without stacktrace would be
sufficient :) `URI.create` shouldn't throw, but we shouldn't crash on bad user
import :)))
done
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732402303
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -21,9 +21,16 @@
import static org.apache.wicket.csp.CSPDirectiveSrcValue.SELF;
import static org.apache.wicket.csp.CSPDirectiveSrcValue.STRICT_DYNAMIC;
+import java.net.URI;
+import java.net.URISyntaxException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
import com.github.openjson.JSONObject;
public class OpenmeetingsVariables {
+ private static final Logger log =
LoggerFactory.getLogger(OpenmeetingsVariables.class);
Review comment:
There is a try/catch block. That should log sth.
https://www.overops.com/blog/swallowed-exceptions-the-silent-killer-of-java-applications/
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732402032
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
Updated to above syntax
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732401983
##
File path:
openmeetings-webservice/src/main/java/org/apache/openmeetings/webservice/InfoWebService.java
##
@@ -106,7 +106,7 @@ public String getManifest() {
manifest.put("name", OpenmeetingsVariables.getApplicationName()
+ " " + Version.getVersion());
manifest.put("short_name",
OpenmeetingsVariables.getApplicationName() + " " + Version.getVersion());
manifest.put("description", "Openmeetings provides video
conferencing, instant messaging, white board, collaborative document editing
and other groupware tools.");
- manifest.put("start_url", "/" +
OpenmeetingsVariables.getWebappPath() + "/?pwa=true");
+ manifest.put("start_url",
OpenmeetingsVariables.getWebappPath() + "?pwa=true");
Review comment:
Updated to use URI.resolve
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732397491
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
also the constructor new URI will not compile without a try/catch block.
Or re-throwing the exception.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732398683
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
I can refactor it a bit, but your proposal doesn't translate that easy
into code. You not even using "baseURL" to construct the URI anymore. Some of
those would just return the same string as passed into.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732398683
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
I can refactor it a bit, but your proposal doesn't translate that easy
into code. You not even using "baseURL" anymore.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732397491
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
also the constructor new URI not compile without a try/catch block. Or
re-throwing the exception.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732397029
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
silent exception / swallowing exceptions is not good. For various
reasons:
https://www.overops.com/blog/swallowed-exceptions-the-silent-killer-of-java-applications/
I think you should at least create a log if you catch an exception.
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #166: OPENMEETINGS-2692 Update review comments on parsing path element.
sebawagner commented on a change in pull request #166:
URL: https://github.com/apache/openmeetings/pull/166#discussion_r732396315
##
File path:
openmeetings-util/src/main/java/org/apache/openmeetings/util/OpenmeetingsVariables.java
##
@@ -249,11 +256,12 @@ public static String getBaseUrl() {
}
public static String getWebappPath() {
- String webappPath = baseUrl;
- if (webappPath.endsWith("/")) {
- webappPath = webappPath.substring(0,
webappPath.length() - 1);
+ try {
Review comment:
shouldn't it log something if there is an exception ?
--
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: dev-unsubscr...@openmeetings.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
