[GitHub] coveralls edited a comment on issue #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui…
coveralls edited a comment on issue #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui… URL: https://github.com/apache/commons-lang/pull/396#issuecomment-451688667 [](https://coveralls.io/builds/20921958) Coverage remained the same at 95.273% when pulling **ead7cc0564bb6135623aade3b661e48bfdd60b72 on PascalSchumacher:travis_java_13** into **e0a5a4b5b63068063878fa68d2568e57d9e1f4fc on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] asfgit closed pull request #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui…
asfgit closed pull request #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui… URL: https://github.com/apache/commons-lang/pull/396 This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/.travis.yml b/.travis.yml index c1b70e3b3..19630c1a9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,10 +18,12 @@ language: java jdk: - oraclejdk8 - openjdk11 + - openjdk12 - openjdk-ea matrix: allow_failures: +- jdk: openjdk12 - jdk: openjdk-ea script: This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] PascalSchumacher opened a new pull request #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui…
PascalSchumacher opened a new pull request #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui… URL: https://github.com/apache/commons-lang/pull/396 …ld on. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] coveralls commented on issue #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui…
coveralls commented on issue #396: Travis: "openjdk-ea" now means Java 13, so add Java 12 to JDKs to bui… URL: https://github.com/apache/commons-lang/pull/396#issuecomment-451688667 [](https://coveralls.io/builds/20921921) Coverage remained the same at 95.273% when pulling **810e73127e4bf288e3877b0ba45feee1a38f56a6 on PascalSchumacher:travis_java_13** into **e0a5a4b5b63068063878fa68d2568e57d9e1f4fc on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] JiangYongKang closed pull request #318: clean code
JiangYongKang closed pull request #318: clean code URL: https://github.com/apache/commons-lang/pull/318 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 commented on a change in pull request #395: Replaces the given String,
with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247473007
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
Review comment:
Thanks a lot,
As per your suggestion have changed the code as follows.
```java
public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
if (str == null) {
return null;
}
if (open == null || close == null) {
return str;
}
final int start = str.indexOf(open);
if (start != INDEX_NOT_FOUND) {
String preceding = "";
if (start > 0) {
preceding = str.substring(0, start);
}
final int end = str.indexOf(close, start + open.length());
if (end != INDEX_NOT_FOUND) {
String exceeding = "";
if ((end < str.length() - 1) && (end + close.length() <
str.length())) {
exceeding = str.substring(end + close.length(),
str.length());
}
//String middleString = str.substring(start + open.length(),
end);
return preceding + open + replace + close + exceeding;
}
}
return str;
}
```
Please suggest that the name of the function is valid
`replaceSubstringInBetween()` or do i need to change the name.
I will add more unit test on this function.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kinow commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
kinow commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-454675700 I am not sure if this belongs to commons-lang. It has a practical use, and I can see you put some effort to document it and write some tests (thanks for that). But being a low level library, I feel like this would be a very simplistic template engine. Don't have time to create a working example, but I suspect I could do that with Apache Commons JEXL, Apache Velocity, or another complete template engine. This looks more related to Apache Commons Text (plus `StringUtils` is quite bloated too), but I would still keep the argument above before merging it into [text]. The replacement part can be done with `StrSubstitutor` from text. The iteration part, that creates multiple `` entries... well, that part I am not sure if we have in Commons Lang or Text something similar. Anyway, if others still want to merge this new feature, it might be a good idea to drop an e-mail to the mailing list to see what others think. Cheers Bruno This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 commented on a change in pull request #395: Replaces the given String,
with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247473007
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
Review comment:
Thanks a lot,
As per your suggestion have changed the code as follows.
```java
public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
if (str == null) {
return null;
}
if (open == null || close == null) {
return str;
}
final int start = str.indexOf(open);
if (start != INDEX_NOT_FOUND) {
String preceding = str.substring(0, start);
final int end = str.indexOf(close, start + open.length());
if (end != INDEX_NOT_FOUND) {
String exceeding = str.substring(end + close.length(),
str.length());
//String middleString = str.substring(start + open.length(),
end);
return preceding + open + replace + close + exceeding;
}
}
return str;
}
```
Please suggest that the name of the function is valid
`replaceSubstringInBetween()` or do i need to change the name.
I will add more unit test on this function.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Yash-777 closed pull request #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 closed pull request #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395 As this is a foreign pull request (from a fork), the diff has been sent to your commit mailing list, comm...@commons.apache.org This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 opened a new pull request #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 opened a new pull request #395: Replaces the given String, with the
String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395
Replace String with the String which is nested in between two Strings.
I have used this Method in order to generate dynamic HTML reports to send a
mail.
```java
public class Test {
public static void main(String[] args) throws Exception {
String data = new
String(Files.readAllBytes(Paths.get("./dynamicReport.html")));
String rowSaperator = "";
String rowDataSaperator = "[~$List$~]";
List dynamicRecords = new ArrayList<>();
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "2", "1" } );
dynamicRecords.add( new String[]{ "1", "2" } );
String finalTableData = dynamicRecordData(data, rowSaperator,
rowDataSaperator, dynamicRecords);
String targetFilePath = "D:/dynamicReport.txt";
try (PrintWriter out = new PrintWriter( targetFilePath )) {
out.println( finalTableData );
}
}
public static String dynamicRecordData(String str, String rowSaperator,
String rowDataSaperator, List dynamicRecords) throws Exception {
if( str.length() <= ( (rowSaperator.length())*2 +
rowDataSaperator.length()) ) {
return str;
}
String substringBetween = StringUtils.substringBetween(str,
rowSaperator, rowSaperator);
if( substringBetween == null) {
return str;
}
int countMatches = StringUtils.countMatches(substringBetween,
rowDataSaperator);
if( str.length() <= ( (rowSaperator.length())*2 +
(rowDataSaperator.length())*countMatches ) ) {
return str;
}
StringBuffer dynamicRowData = new StringBuffer();
String[] searchList = null;
for (String[] dynamicRecord : dynamicRecords) {
if( dynamicRecord.length == countMatches ) {
if( searchList == null ) {
searchList = new String[countMatches];
for (int i = 0; i < countMatches; i++) {
searchList[i] = rowDataSaperator;
}
}
String replaceEach =
StringUtils.replaceEach(substringBetween, searchList, dynamicRecord);
dynamicRowData.append(replaceEach);
} else {
throw new Exception("Records mismatch.");
}
}
String finalTableData = StringUtils.replaceSubstringInBetween(str,
dynamicRowData.toString(), rowSaperator, rowSaperator);
return finalTableData;
}
}
```
Sample HTML Template. Where i my case there are to many dynamic tables with
provided list data to replace.
```html
Header1Header2
[~$List$~][~$List$~]
```
After code generated report.
```html
Header1Header2
11
11
22
11
```
Avoid previous pull requests [`393`,
`394`](https://travis-ci.org/apache/commons-lang/pull_requests)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Yash-777 closed pull request #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 closed pull request #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395 As this is a foreign pull request (from a fork), the diff has been sent to your commit mailing list, comm...@commons.apache.org This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-454680834 creating new pull request. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] coveralls edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings.
coveralls edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-451407579 [](https://coveralls.io/builds/21099947) Coverage decreased (-0.08%) to 95.198% when pulling **bd50690a1b622622d1010b741279ec9f1133358d on Yash-777:master** into **e0a5a4b5b63068063878fa68d2568e57d9e1f4fc on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r248161742
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
Review comment:
As is, you would still need to account for negative indices being returned
from `str.indexOf(open) `and `str.indexOf(close, start + open.length())`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kinow commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
kinow commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-454682674 @Yash-777 I think it would be easier if you used the same pull request. This makes it a lot harder to review, especially as all the previous comments from other devs will be lost on your new pull request. You can edit commits, or push new commits to your own branch, and that should update the pull request automatically. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] coveralls commented on issue #400: Close/flush the `OutputStream` before calling `toByteArray()` on underlying `ByteArrayOutputStream`
coveralls commented on issue #400: Close/flush the `OutputStream` before calling `toByteArray()` on underlying `ByteArrayOutputStream` URL: https://github.com/apache/commons-lang/pull/400#issuecomment-456718148 [](https://coveralls.io/builds/21225999) Coverage remained the same at 95.273% when pulling **3070ab66af7a238428f4ef8b3477ecb1e272cfcf on emopers:flushViolationFixes** into **b00e65686eb1b11c0b814c4aa0d6a22c03be958c on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] emopers opened a new pull request #399: Synchronize before looping over synchronized collection
emopers opened a new pull request #399: Synchronize before looping over synchronized collection URL: https://github.com/apache/commons-lang/pull/399 In `CharSet.java`, `set = Collections.synchronizedSet(new HashSet<>())` is iterated over without synchronization. According to [Java API] (https://docs.oracle.com/javase/6/docs/api/java/util/Collections.html#synchronizedCollection%28java.util.Collection%29), it is recommended that user manually synchronizes each iteration over the synchronized collection to reduce non-determinism. This pull request improves the condition by synchronizing a loop which loops over the synchronized collection. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] emopers opened a new pull request #400: Close/flush the `OutputStream` before calling `toByteArray()` on underlying `ByteArrayOutputStream`
emopers opened a new pull request #400: Close/flush the `OutputStream` before calling `toByteArray()` on underlying `ByteArrayOutputStream` URL: https://github.com/apache/commons-lang/pull/400 When an `OutputStream` instance wraps an underlying `ByteArrayOutputStream` instance, it is recommended to flush or close the `OutputStream` before invoking the underlying instances' `toByteArray()`. Although in some of these case it is not strictly necessary because the `writeObject()` method is invoked right before `toByteArray()`, and `writeObject()` internally calls `flush()`/`drain()`. However, it is good practice to call `flush()`/`close()` explicitly as mentioned, for example, [here](http://stackoverflow.com/questions/2984538/how-to-use-bytearrayoutputstream-and-dataoutputstream-simultaneously-java). This pull request adds a call to `close()` or `flush()` before calls to `toByteArray()`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] coveralls commented on issue #399: Synchronize before looping over synchronized collection
coveralls commented on issue #399: Synchronize before looping over synchronized collection URL: https://github.com/apache/commons-lang/pull/399#issuecomment-456717202 [](https://coveralls.io/builds/21225943) Coverage decreased (-0.01%) to 95.261% when pulling **34398f520abdb2b973ce417d5f1982909157ec08 on emopers:syncViolationFix** into **b00e65686eb1b11c0b814c4aa0d6a22c03be958c on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Sam-Kruglov opened a new pull request #398: Add ComparableUtils
Sam-Kruglov opened a new pull request #398: Add ComparableUtils URL: https://github.com/apache/commons-lang/pull/398 I have turned `feeAmount.compareTo(BigDecimal.ZERO) > 0` into `is(feeAmount).greaterThan(BigDecimal.ZERO)` and decided to share. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 opened a new pull request #397: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 opened a new pull request #397: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/397 This method returns the input String on the below conditions: * If the input String is null * if open`|`close`|`replace Strings are null Added to pull [`#395`](https://github.com/apache/commons-lang/pull/395) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] coveralls commented on issue #397: Replaces the given String, with the String which is nested in between two Strings.
coveralls commented on issue #397: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/397#issuecomment-454686192 [](https://coveralls.io/builds/21100151) Coverage decreased (-0.08%) to 95.198% when pulling **aef4d75c461e0d3f4d667fcaf678d66c38f0c7f1 on Yash-777:master** into **b00e65686eb1b11c0b814c4aa0d6a22c03be958c on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] coveralls commented on issue #398: Add ComparableUtils
coveralls commented on issue #398: Add ComparableUtils URL: https://github.com/apache/commons-lang/pull/398#issuecomment-455218326 [](https://coveralls.io/builds/21131417) Coverage decreased (-0.003%) to 95.27% when pulling **cf07c2b29d3c27bead181b4433fa499fe5ed885a on Sam-Kruglov:LANG-1431-ComparableUtils** into **b00e65686eb1b11c0b814c4aa0d6a22c03be958c on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on issue #398: Add ComparableUtils
MarkDacek commented on issue #398: Add ComparableUtils URL: https://github.com/apache/commons-lang/pull/398#issuecomment-455432213 I think you'd have to add in null-safety to make this worthwhile. Without that, it's just overhead on top of `a.compareTo(b)`. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Sam-Kruglov commented on issue #398: Add ComparableUtils
Sam-Kruglov commented on issue #398: Add ComparableUtils URL: https://github.com/apache/commons-lang/pull/398#issuecomment-455475212 @MarkDacek @GedMarc actually that's correct, compareTo is null safe, here is a piece of its javadoc: `@throws NullPointerException if the specified object is null` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] GedMarc commented on issue #398: Add ComparableUtils
GedMarc commented on issue #398: Add ComparableUtils URL: https://github.com/apache/commons-lang/pull/398#issuecomment-455463754 @MarkDacek compareTo() is null safe by default, that's the difference vs compare(T a1,T a2)? So adding in null checks into compareTo() seems likes a waste of overhead? Not sure if i'm misunderstanding this - http://www.javapractices.com/topic/TopicAction.do?Id=10 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Sam-Kruglov commented on issue #398: Add ComparableUtils
Sam-Kruglov commented on issue #398: Add ComparableUtils URL: https://github.com/apache/commons-lang/pull/398#issuecomment-455448673 Good idea will add. But following that logic, the whole commons is just an overhead then. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value
MarkDacek commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value URL: https://github.com/apache/commons-lang/pull/362#issuecomment-448013754 Can you think of any kind of use-case where a String over two-billion characters long is created, such that all other calls to this should check for it? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on issue #391: Adding junits for JsonToStringStyle
MarkDacek commented on issue #391: Adding junits for JsonToStringStyle URL: https://github.com/apache/commons-lang/pull/391#issuecomment-448004147 Can you make your arrays in the new test cases final? Also perhaps avoid using the **new** keyword when you don't need memory allocated at that exact moment. Paging @chtompki for context. Should the JSON blurbs that are the expected output be final Strings as well? That's not the existing pattern, but it seems a bit odd to be concatenating to make deterministic Strings during an assert. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Turan91 commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value
Turan91 commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value URL: https://github.com/apache/commons-lang/pull/362#issuecomment-448012492 It is convention for exception messages to provide the user with information on the cause of failure. In the same way an ArrayIndexOutOfBoundsException would provide the user with the length which caused it even though the "developer would clearly have access to that information." This provides the user with information on the offending value. With regards to overhead it is a single check at the start that doesn't add much overhead and prevents issues with both being too large and failing silently (with regards to the expected string not being what the users expects as a result). This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r242326099
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
I tend to agree with sz being more appropriate of a name here than strLen.
It would appear that no other variable named `strLen` ever deviates from its
original assignment (even the non-final ones.)
Saving a loop variable isn't a great gain in memory usage but I suppose this
is a bit more appropriate. Though, thinking ahead for the future - can one
imagine having to reference the string length for a future change? That would
be my opposition for this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value
MarkDacek commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value URL: https://github.com/apache/commons-lang/pull/362#issuecomment-448005772 Other thing of note: I don't think that providing the length in the exception is terribly useful, as the developer would clearly have access to that information. Is there consensus on whether this is a needed feature? Is there a JIRA ticket open for this? I am having a hard time believing that multi-billion character Strings emanating from this method are common enough to warrant the overhead. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r242326701
##
File path:
src/test/java/org/apache/commons/lang3/StringUtilsEmptyBlankTest.java
##
@@ -99,6 +110,7 @@ public void testIsNotBlank() {
assertFalse(StringUtils.isNotBlank(null));
assertFalse(StringUtils.isNotBlank(""));
assertFalse(StringUtils.isNotBlank(StringUtilsTest.WHITESPACE));
+assertTrue(StringUtils.isNotBlank("a"));
Review comment:
I might be missing something, but this appears redundant with the next line.
Did this add any coverage?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rosti-il commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
rosti-il commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r242420905
##
File path:
src/test/java/org/apache/commons/lang3/StringUtilsEmptyBlankTest.java
##
@@ -99,6 +110,7 @@ public void testIsNotBlank() {
assertFalse(StringUtils.isNotBlank(null));
assertFalse(StringUtils.isNotBlank(""));
assertFalse(StringUtils.isNotBlank(StringUtilsTest.WHITESPACE));
+assertTrue(StringUtils.isNotBlank("a"));
Review comment:
This is a special case that fails if you accidentally use `index > 0`
instead of `index >= 0` in the expression of the down going loop. The next line
will not fail in this case.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rosti-il commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
rosti-il commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r242423165
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
@MarkDacek, this and also the `isWhitespace()` methods are just string scan
methods. They don't need the string length except the very beginning. Thus the
original value returned by `cs.length()` doesn't need to be memorized.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] chtompki commented on issue #391: Adding junits for JsonToStringStyle
chtompki commented on issue #391: Adding junits for JsonToStringStyle URL: https://github.com/apache/commons-lang/pull/391#issuecomment-448218634 I'm fairly indifferent about how the strings are declared as they are tests. The concatenation is a tad odd to me because it seems to be unnecessary. That said, I'm all for more tests. I say we pull this in and clean it up in `master` because having more tests is always a good thing. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] chtompki commented on issue #391: Adding junits for JsonToStringStyle
chtompki commented on issue #391: Adding junits for JsonToStringStyle URL: https://github.com/apache/commons-lang/pull/391#issuecomment-448221818 I'll try to get to this today. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r242667471
##
File path:
src/test/java/org/apache/commons/lang3/StringUtilsEmptyBlankTest.java
##
@@ -99,6 +110,7 @@ public void testIsNotBlank() {
assertFalse(StringUtils.isNotBlank(null));
assertFalse(StringUtils.isNotBlank(""));
assertFalse(StringUtils.isNotBlank(StringUtilsTest.WHITESPACE));
+assertTrue(StringUtils.isNotBlank("a"));
Review comment:
Might be worth adding a comment in there for that.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] RahulNagekar commented on issue #391: Adding junits for JsonToStringStyle
RahulNagekar commented on issue #391: Adding junits for JsonToStringStyle URL: https://github.com/apache/commons-lang/pull/391#issuecomment-449103250 Will post 1 more commit improving this. I am all agree with all comnents , its best to follow best practices. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rosti-il commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
rosti-il commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r243349505
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
So what name do you think is better in this case? `index`, `idx` or
something else?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] eceejcr commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
eceejcr commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r243382598
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
idx is fine for me
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek edited a comment on issue #362: Add a check to StringUtils.repeat() for large length repeat value
MarkDacek edited a comment on issue #362: Add a check to StringUtils.repeat() for large length repeat value URL: https://github.com/apache/commons-lang/pull/362#issuecomment-447725344 > > I'm somewhat wary of simply comparing the values, as it's not always intuitive and doesn't save us much in the way of performance. Perhaps comparing it to Integer.MAX_VALUE is a bit more appropriate? > > What do you mean? https://github.com/apache/commons-lang/pull/362/files#diff-3f4c835875ddc8a211c0272e8be51394R6253 I would sooner do something like: `final long longSize = (long) inputLength * repeat;` `if(longSize > Integer.MAX_VALUE)` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value
MarkDacek commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value URL: https://github.com/apache/commons-lang/pull/362#issuecomment-447725344 > > I'm somewhat wary of simply comparing the values, as it's not always intuitive and doesn't save us much in the way of performance. Perhaps comparing it to Integer.MAX_VALUE is a bit more appropriate? > > What do you mean? https://github.com/apache/commons-lang/pull/362/files#diff-3f4c835875ddc8a211c0272e8be51394R6253 I would sooner do something like: `final long longSize = (long) inputLength * (long) repeat;` `if(longSize > Integer.MAX_VALUE)` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Turan91 commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value
Turan91 commented on issue #362: Add a check to StringUtils.repeat() for large length repeat value URL: https://github.com/apache/commons-lang/pull/362#issuecomment-449603524 Validation checks are validation checks, we can't begin using occurrence to dictate their place. As with any utility a set number of validation checks are performed before the actual brunt of the logic. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rosti-il commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
rosti-il commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r242720230
##
File path:
src/test/java/org/apache/commons/lang3/StringUtilsEmptyBlankTest.java
##
@@ -99,6 +110,7 @@ public void testIsNotBlank() {
assertFalse(StringUtils.isNotBlank(null));
assertFalse(StringUtils.isNotBlank(""));
assertFalse(StringUtils.isNotBlank(StringUtilsTest.WHITESPACE));
+assertTrue(StringUtils.isNotBlank("a"));
Review comment:
Done in the second commit.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rosti-il commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
rosti-il commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r242720230
##
File path:
src/test/java/org/apache/commons/lang3/StringUtilsEmptyBlankTest.java
##
@@ -99,6 +110,7 @@ public void testIsNotBlank() {
assertFalse(StringUtils.isNotBlank(null));
assertFalse(StringUtils.isNotBlank(""));
assertFalse(StringUtils.isNotBlank(StringUtilsTest.WHITESPACE));
+assertTrue(StringUtils.isNotBlank("a"));
Review comment:
Done in second commit.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] eceejcr commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
eceejcr commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r243179720
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
Regarding the final keyword for ‘sz’, in my view makes sense to make the
code uniform and in general, the final keyword is used in the surrounding
affected for variables non-mutating (even for variables inside methods).
Personally, I like it because think it makes the code more readable providing
information about the mutability of variable
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Yash-777 edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-452188188 Already i have closed `393, 394` pull requests. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] JiangYongKang removed a comment on issue #318: clean code
JiangYongKang removed a comment on issue #318: clean code URL: https://github.com/apache/commons-lang/pull/318#issuecomment-399720458 @stokito thinks This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-451852546 Can any one explain that, Is the build fails because of an extra added function **`replaceSubstringInBetween()`** in the class `StringUtils`. > #1454.2 ```log [INFO] Running org.apache.commons.lang3.RandomStringUtilsTest [ERROR] Tests run: 18, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.226 s <<< FAILURE! - in org.apache.commons.lang3.RandomStringUtilsTest [ERROR] testRandomStringUtilsHomog Time elapsed: 0.019 s <<< FAILURE! org.opentest4j.AssertionFailedError: test homogeneity -- will fail about 1 in 1000 times ==> expected: but was: at org.apache.commons.lang3.RandomStringUtilsTest.testRandomStringUtilsHomog(RandomStringUtilsTest.java:464) ``` > #1454.3 ```log [ERROR] Failures: [ERROR] FieldUtilsTest.testRemoveFinalModifier:998 expected: but was: [ERROR] FieldUtilsTest.testRemoveFinalModifierWithAccess:1009 expected: but was: ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ecki commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
ecki commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-452019406 The test States `will fail about 1 in 1000 times` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ecki commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
ecki commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-452019913 Can you please merge your commits to a single one, so it is easier to review and apply. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-452188188 Already i have closed the pull requests `393, 394` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] kinow commented on issue #379: add tests for use ImmutableTriple as key in java.util.HashMap and java.util.TreeMap
kinow commented on issue #379: add tests for use ImmutableTriple as key in java.util.HashMap and java.util.TreeMap URL: https://github.com/apache/commons-lang/pull/379#issuecomment-452194089 Merged in https://github.com/apache/commons-lang/commit/bf7767867378d9d33551ad54687b48c8fbecf424 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] kinow closed pull request #379: add tests for use ImmutableTriple as key in java.util.HashMap and java.util.TreeMap
kinow closed pull request #379: add tests for use ImmutableTriple as key in
java.util.HashMap and java.util.TreeMap
URL: https://github.com/apache/commons-lang/pull/379
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/src/test/java/org/apache/commons/lang3/tuple/ImmutablePairTest.java
b/src/test/java/org/apache/commons/lang3/tuple/ImmutablePairTest.java
index 260879a95..a5b0d9d20 100644
--- a/src/test/java/org/apache/commons/lang3/tuple/ImmutablePairTest.java
+++ b/src/test/java/org/apache/commons/lang3/tuple/ImmutablePairTest.java
@@ -26,6 +26,11 @@
import java.io.ByteArrayOutputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map.Entry;
+import java.util.TreeMap;
import org.junit.jupiter.api.Test;
@@ -136,4 +141,34 @@ public void testSerialization() throws Exception {
assertEquals(origPair, deserializedPair);
assertEquals(origPair.hashCode(), deserializedPair.hashCode());
}
+
+@Test
+public void testUseAsKeyOfHashMap() {
+HashMap, String> map = new HashMap<>();
+Object o1 = new Object();
+Object o2 = new Object();
+ImmutablePair key1 = ImmutablePair.of(o1, o2);
+String value1 = "a1";
+map.put(key1, value1);
+assertEquals(value1, map.get(key1));
+assertEquals(value1, map.get(ImmutablePair.of(o1, o2)));
+}
+
+@Test
+public void testUseAsKeyOfTreeMap() {
+TreeMap, String> map = new TreeMap<>();
+map.put(ImmutablePair.of(1, 2), "12");
+map.put(ImmutablePair.of(1, 1), "11");
+map.put(ImmutablePair.of(0, 1), "01");
+ArrayList> expected = new
ArrayList<>();
+expected.add(ImmutablePair.of(0, 1));
+expected.add(ImmutablePair.of(1, 1));
+expected.add(ImmutablePair.of(1, 2));
+Iterator, String>> it =
map.entrySet().iterator();
+for(ImmutablePair item : expected) {
+Entry, String> entry = it.next();
+assertEquals(item, entry.getKey());
+assertEquals(item.getLeft() + "" + item.getRight(),
entry.getValue());
+}
+}
}
diff --git
a/src/test/java/org/apache/commons/lang3/tuple/ImmutableTripleTest.java
b/src/test/java/org/apache/commons/lang3/tuple/ImmutableTripleTest.java
index 8f76000bd..fe73930bb 100644
--- a/src/test/java/org/apache/commons/lang3/tuple/ImmutableTripleTest.java
+++ b/src/test/java/org/apache/commons/lang3/tuple/ImmutableTripleTest.java
@@ -26,6 +26,11 @@
import java.io.ByteArrayOutputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.TreeMap;
+import java.util.Map.Entry;
import org.junit.jupiter.api.Test;
@@ -142,5 +147,36 @@ public void testSerialization() throws Exception {
assertEquals(origTriple, deserializedTriple);
assertEquals(origTriple.hashCode(), deserializedTriple.hashCode());
}
+
+@Test
+public void testUseAsKeyOfHashMap() {
+HashMap, String> map = new
HashMap<>();
+Object o1 = new Object();
+Object o2 = new Object();
+Object o3 = new Object();
+ImmutableTriple key1 = ImmutableTriple.of(o1,
o2, o3);
+String value1 = "a1";
+map.put(key1, value1);
+assertEquals(value1, map.get(key1));
+assertEquals(value1, map.get(ImmutableTriple.of(o1, o2, o3)));
+}
+
+@Test
+public void testUseAsKeyOfTreeMap() {
+TreeMap, String> map = new
TreeMap<>();
+map.put(ImmutableTriple.of(0, 1, 2), "012");
+map.put(ImmutableTriple.of(0, 1, 1), "011");
+map.put(ImmutableTriple.of(0, 0, 1), "001");
+ArrayList> expected = new
ArrayList<>();
+expected.add(ImmutableTriple.of(0, 0, 1));
+expected.add(ImmutableTriple.of(0, 1, 1));
+expected.add(ImmutableTriple.of(0, 1, 2));
+Iterator, String>> it
= map.entrySet().iterator();
+for(ImmutableTriple item : expected) {
+Entry, String> entry =
it.next();
+assertEquals(item, entry.getKey());
+assertEquals(item.getLeft() + "" + item.getMiddle() + "" +
item.getRight(), entry.getValue());
+}
+}
}
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] kinow commented on issue #379: add tests for use ImmutableTriple as key in java.util.HashMap and java.util.TreeMap
kinow commented on issue #379: add tests for use ImmutableTriple as key in java.util.HashMap and java.util.TreeMap URL: https://github.com/apache/commons-lang/pull/379#issuecomment-452193708 Generated site from `master` branch with `-Pjacoco`. Then stored the site elsewhere. Checked out pull request locally. Squashed commits into a single one. Then rebased onto `master`. Generated site again, now from the pull request, and confirmed coverage went up, and build still passing OK. Code looks OK, so merging. Thanks @apirom9 ! Bruno This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-451852546 Can any one explain that, the build is fail because of extra added function **`replaceSubstringInBetween`** in class name `StringUtils`. > #1454.2 ```log [INFO] Running org.apache.commons.lang3.RandomStringUtilsTest [ERROR] Tests run: 18, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.226 s <<< FAILURE! - in org.apache.commons.lang3.RandomStringUtilsTest [ERROR] testRandomStringUtilsHomog Time elapsed: 0.019 s <<< FAILURE! org.opentest4j.AssertionFailedError: test homogeneity -- will fail about 1 in 1000 times ==> expected: but was: at org.apache.commons.lang3.RandomStringUtilsTest.testRandomStringUtilsHomog(RandomStringUtilsTest.java:464) ``` > #1454.2 ```log [ERROR] Failures: [ERROR] FieldUtilsTest.testRemoveFinalModifier:998 expected: but was: [ERROR] FieldUtilsTest.testRemoveFinalModifierWithAccess:1009 expected: but was: ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 edited a comment on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-451852546 Can any one explain that, the build is fail because of extra added function **`replaceSubstringInBetween`** in class name `StringUtils`. > #1454.2 ```log [INFO] Running org.apache.commons.lang3.RandomStringUtilsTest [ERROR] Tests run: 18, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.226 s <<< FAILURE! - in org.apache.commons.lang3.RandomStringUtilsTest [ERROR] testRandomStringUtilsHomog Time elapsed: 0.019 s <<< FAILURE! org.opentest4j.AssertionFailedError: test homogeneity -- will fail about 1 in 1000 times ==> expected: but was: at org.apache.commons.lang3.RandomStringUtilsTest.testRandomStringUtilsHomog(RandomStringUtilsTest.java:464) ``` > #1454.3 ```log [ERROR] Failures: [ERROR] FieldUtilsTest.testRemoveFinalModifier:998 expected: but was: [ERROR] FieldUtilsTest.testRemoveFinalModifierWithAccess:1009 expected: but was: ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 closed pull request #394: Replace String for reports
Yash-777 closed pull request #394: Replace String for reports
URL: https://github.com/apache/commons-lang/pull/394
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/src/main/java/org/apache/commons/lang3/StringUtils.java
b/src/main/java/org/apache/commons/lang3/StringUtils.java
index 7fcd0246e..0f5f0fc1d 100644
--- a/src/main/java/org/apache/commons/lang3/StringUtils.java
+++ b/src/main/java/org/apache/commons/lang3/StringUtils.java
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
+}
+final int end = str.indexOf(close, start + open.length());
+if (end != INDEX_NOT_FOUND) {
+String exceding = "";
+if ((end < str.length() - 1) && (end + close.length() <
str.length())) {
+exceding = str.substring(end + close.length(),
str.length());
+}
+//String middleString = str.substring(start + open.length(),
end);
+return preceding + open + replace + close + exceding;
+}
+}
+return null;
+}
// Nested extraction
//---
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Yash-777 opened a new pull request #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 opened a new pull request #395: Replaces the given String, with the
String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395
Replace String with the String which is nested in between two Strings.
I have used this Method in order to generate dynamic HTML reports to send a
mail.
```java
public class Test {
public static void main(String[] args) throws Exception {
String data = new
String(Files.readAllBytes(Paths.get("./dynamicReport.html")));
String rowSaperator = "";
String rowDataSaperator = "[~$List$~]";
List dynamicRecords = new ArrayList<>();
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "2", "1" } );
dynamicRecords.add( new String[]{ "1", "2" } );
String finalTableData = dynamicRecordData(data, rowSaperator,
rowDataSaperator, dynamicRecords);
String targetFilePath = "D:/dynamicReport.txt";
try (PrintWriter out = new PrintWriter( targetFilePath )) {
out.println( finalTableData );
}
}
public static String dynamicRecordData(String str, String rowSaperator,
String rowDataSaperator, List dynamicRecords) throws Exception {
if( str.length() <= ( (rowSaperator.length())*2 +
rowDataSaperator.length()) ) {
return str;
}
String substringBetween = StringUtils.substringBetween(str,
rowSaperator, rowSaperator);
if( substringBetween == null) {
return str;
}
int countMatches = StringUtils.countMatches(substringBetween,
rowDataSaperator);
if( str.length() <= ( (rowSaperator.length())*2 +
(rowDataSaperator.length())*countMatches ) ) {
return str;
}
StringBuffer dynamicRowData = new StringBuffer();
String[] searchList = null;
for (String[] dynamicRecord : dynamicRecords) {
if( dynamicRecord.length == countMatches ) {
if( searchList == null ) {
searchList = new String[countMatches];
for (int i = 0; i < countMatches; i++) {
searchList[i] = rowDataSaperator;
}
}
String replaceEach =
StringUtils.replaceEach(substringBetween, searchList, dynamicRecord);
dynamicRowData.append(replaceEach);
} else {
throw new Exception("Records mismatch.");
}
}
String finalTableData =
TabularData_from_List.replaceSubstringInBetween(str, dynamicRowData.toString(),
rowSaperator, rowSaperator);
return finalTableData;
}
}
```
Sample HTML Template. Where i my case there are to many dynamic tables with
provided list data to replace.
```html
Header1Header2
[~$List$~][~$List$~]
```
After code generated report.
```html
Header1Header2
11
11
22
11
```
Avoid previous pull requests `393`, `394`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] coveralls commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
coveralls commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-451407579 [](https://coveralls.io/builds/20902091) Coverage decreased (-0.09%) to 95.185% when pulling **8e5aedfac26714f1a9c520f52c9a7b7f0681c193 on Yash-777:master** into **e0a5a4b5b63068063878fa68d2568e57d9e1f4fc on apache:master**. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 closed pull request #393: Replace String
Yash-777 closed pull request #393: Replace String
URL: https://github.com/apache/commons-lang/pull/393
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/src/main/java/org/apache/commons/lang3/StringUtils.java
b/src/main/java/org/apache/commons/lang3/StringUtils.java
index 7fcd0246e..47242f800 100644
--- a/src/main/java/org/apache/commons/lang3/StringUtils.java
+++ b/src/main/java/org/apache/commons/lang3/StringUtils.java
@@ -3124,6 +3124,43 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
+}
+
+final int end = str.indexOf(close, start + open.length());
+if (end != INDEX_NOT_FOUND) {
+String exceding = "";
+if ((end < str.length() - 1) && (end + close.length() <
str.length())) {
+exceding = str.substring(end + close.length(),
str.length());
+}
+//String middleString = str.substring(start + open.length(),
end);
+return preceding + open + replace + close + exceding;
+}
+}
+return null;
+}
+
// Nested extraction
//---
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Yash-777 commented on issue #393: Replace String
Yash-777 commented on issue #393: Replace String URL: https://github.com/apache/commons-lang/pull/393#issuecomment-451380562 I will create new pull request after solving the 7 Checkstyle violations. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] Yash-777 opened a new pull request #393: Replace String
Yash-777 opened a new pull request #393: Replace String
URL: https://github.com/apache/commons-lang/pull/393
Replace String with the String which is nested in between two Strings.
I have used this Method in order to generate dynamic HTML reports to send a
mail.
```java
public class Test {
public static void main(String[] args) throws Exception {
String data = new
String(Files.readAllBytes(Paths.get("./dynamicReport.html")));
String rowSaperator = "";
String rowDataSaperator = "[~$List$~]";
List dynamicRecords = new ArrayList<>();
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "2", "1" } );
dynamicRecords.add( new String[]{ "1", "2" } );
String finalTableData = dynamicRecordData(data, rowSaperator,
rowDataSaperator, dynamicRecords);
String targetFilePath = "D:/dynamicReport.txt";
try (PrintWriter out = new PrintWriter( targetFilePath )) {
out.println( finalTableData );
}
}
public static String dynamicRecordData(String str, String rowSaperator,
String rowDataSaperator, List dynamicRecords) throws Exception {
if( str.length() <= ( (rowSaperator.length())*2 +
rowDataSaperator.length()) ) {
return str;
}
String substringBetween = StringUtils.substringBetween(str,
rowSaperator, rowSaperator);
if( substringBetween == null) {
return str;
}
int countMatches = StringUtils.countMatches(substringBetween,
rowDataSaperator);
if( str.length() <= ( (rowSaperator.length())*2 +
(rowDataSaperator.length())*countMatches ) ) {
return str;
}
StringBuffer dynamicRowData = new StringBuffer();
String[] searchList = null;
for (String[] dynamicRecord : dynamicRecords) {
if( dynamicRecord.length == countMatches ) {
if( searchList == null ) {
searchList = new String[countMatches];
for (int i = 0; i < countMatches; i++) {
searchList[i] = rowDataSaperator;
}
}
String replaceEach =
StringUtils.replaceEach(substringBetween, searchList, dynamicRecord);
dynamicRowData.append(replaceEach);
} else {
throw new Exception("Records mismatch.");
}
}
String finalTableData =
TabularData_from_List.replaceSubstringInBetween(str, dynamicRowData.toString(),
rowSaperator, rowSaperator);
return finalTableData;
}
}
```
Sample HTML Template. Where i my case there are to many dynamic tables with
provided list data to replace.
```html
Header1Header2
[~$List$~][~$List$~]
```
After code generated report.
```html
Header1Header2
11
11
22
11
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Yash-777 opened a new pull request #394: Replace String for reports
Yash-777 opened a new pull request #394: Replace String for reports
URL: https://github.com/apache/commons-lang/pull/394
Replace String with the String which is nested in between two Strings.
I have used this Method in order to generate dynamic HTML reports to send a
mail.
```java
public class Test {
public static void main(String[] args) throws Exception {
String data = new
String(Files.readAllBytes(Paths.get("./dynamicReport.html")));
String rowSaperator = "";
String rowDataSaperator = "[~$List$~]";
List dynamicRecords = new ArrayList<>();
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "1", "1" } );
dynamicRecords.add( new String[]{ "2", "1" } );
dynamicRecords.add( new String[]{ "1", "2" } );
String finalTableData = dynamicRecordData(data, rowSaperator,
rowDataSaperator, dynamicRecords);
String targetFilePath = "D:/dynamicReport.txt";
try (PrintWriter out = new PrintWriter( targetFilePath )) {
out.println( finalTableData );
}
}
public static String dynamicRecordData(String str, String rowSaperator,
String rowDataSaperator, List dynamicRecords) throws Exception {
if( str.length() <= ( (rowSaperator.length())*2 +
rowDataSaperator.length()) ) {
return str;
}
String substringBetween = StringUtils.substringBetween(str,
rowSaperator, rowSaperator);
if( substringBetween == null) {
return str;
}
int countMatches = StringUtils.countMatches(substringBetween,
rowDataSaperator);
if( str.length() <= ( (rowSaperator.length())*2 +
(rowDataSaperator.length())*countMatches ) ) {
return str;
}
StringBuffer dynamicRowData = new StringBuffer();
String[] searchList = null;
for (String[] dynamicRecord : dynamicRecords) {
if( dynamicRecord.length == countMatches ) {
if( searchList == null ) {
searchList = new String[countMatches];
for (int i = 0; i < countMatches; i++) {
searchList[i] = rowDataSaperator;
}
}
String replaceEach =
StringUtils.replaceEach(substringBetween, searchList, dynamicRecord);
dynamicRowData.append(replaceEach);
} else {
throw new Exception("Records mismatch.");
}
}
String finalTableData =
TabularData_from_List.replaceSubstringInBetween(str, dynamicRowData.toString(),
rowSaperator, rowSaperator);
return finalTableData;
}
}
```
Sample HTML Template. Where i my case there are to many dynamic tables with
provided list data to replace.
```html
Header1Header2
[~$List$~][~$List$~]
```
After code generated report.
```html
Header1Header2
11
11
22
11
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247361985
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
+}
+final int end = str.indexOf(close, start + open.length());
+if (end != INDEX_NOT_FOUND) {
+String exceding = "";
+if ((end < str.length() - 1) && (end + close.length() <
str.length())) {
Review comment:
I'm lost on why this `if` is needed. I would think you'd just call the
`str.substring(end)` and add together `preceeding, open, replace, end`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247361985
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
+}
+final int end = str.indexOf(close, start + open.length());
+if (end != INDEX_NOT_FOUND) {
+String exceding = "";
+if ((end < str.length() - 1) && (end + close.length() <
str.length())) {
Review comment:
I'm lost on why this `if` is needed. I would think you'd just call the
`str.substring(end)` and add together `preceding, open, replace, end`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247361944
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
Review comment:
Based on your code, this is "the String to replace the inner substring".
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247362123
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
Review comment:
Similar to my comment below, I'd think it wise to simplify.
`String preceding = str.substring(0, start + open.length());`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on issue #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on issue #395: Replaces the given String, with the String which is nested in between two Strings. URL: https://github.com/apache/commons-lang/pull/395#issuecomment-453863749 For one, the default return value should be the original `str`, not null. That way, it would be much simpler to use it safely, e.g.: `list.add(StringUtils.replaceSubstringInBetween(args..))` instead of having to add in a null check before adding it. Please add sufficient unit-tests to show that this functions as expected. I would also suggest that a boolean `repeat` be added, to allow this to be done indefinitely. This might require a slight refactor. Other comments to be made inline. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247362069
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
Review comment:
also, `Sting`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247362055
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
+}
+final int end = str.indexOf(close, start + open.length());
+if (end != INDEX_NOT_FOUND) {
+String exceding = "";
+if ((end < str.length() - 1) && (end + close.length() <
str.length())) {
+exceding = str.substring(end + close.length(),
str.length());
+}
+//String middleString = str.substring(start + open.length(),
end);
Review comment:
please remove
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
MarkDacek commented on a change in pull request #395: Replaces the given
String, with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247362048
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
+}
+final int end = str.indexOf(close, start + open.length());
+if (end != INDEX_NOT_FOUND) {
+String exceding = "";
Review comment:
exceeding
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zmacomber commented on issue #358: ClassUtils.getBaseClasses(desiredBase, packageName)
zmacomber commented on issue #358: ClassUtils.getBaseClasses(desiredBase, packageName) URL: https://github.com/apache/commons-lang/pull/358#issuecomment-450391993 Have not heard anything on this PR for 2 months. Should I just close this PR? This doesn't seem like the way open source should work with such a long time for feedback. I'm fine with feedback like, "This is not going to fit in our codebase". This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ecki commented on a change in pull request #358: ClassUtils.getBaseClasses(desiredBase, packageName)
ecki commented on a change in pull request #358:
ClassUtils.getBaseClasses(desiredBase, packageName)
URL: https://github.com/apache/commons-lang/pull/358#discussion_r244365437
##
File path: src/main/java/org/apache/commons/lang3/ClassUtils.java
##
@@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class cls) {
return getClass(loader, className, initialize);
}
+/**
+ * Returns a list of base classes/interfaces underneath the supplied
package
+ * This method only retrieves base classes/interfaces that have children
that can be instantiated
+ * via a no-args constructor
+ * The class loader is retrieved via
Thread.currentThread().getContextClassLoader()
+ * This only retrieves base classes/interfaces directly underneath the
supplied package
+ *
+ * @param desiredBase the desired base class/interface to retrieve
+ * @param packageName the package name in the standard import format (i.e.
"java.lang.String")
+ * @param The desired base class or interface type to retrieve
+ * @return a list of base classes/interfaces that match the supplied type
underneath the supplied package
+ * @throws IllegalArgumentException if the desiredBase or packageName are
invalid
+ * @throws IOException if an I/O error occurs in getting a new directory
stream
+ * @throws NullPointerException if desiredBase or url are null
+ * @throws URISyntaxException if the generated url can't be converted to a
URI
+ */
+public static List getBaseClasses(final Class desiredBase, final
String packageName)
+throws IllegalArgumentException, IOException,
NullPointerException, URISyntaxException {
+
+Objects.requireNonNull(desiredBase, "desiredBase must not be null");
+
+if (StringUtils.isBlank(packageName)) {
+throw new IllegalArgumentException("packageName must not be
blank");
+}
+
+ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
+URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
+Objects.requireNonNull(url, "supplied package not found");
+
+Path classesPath = Paths.get(url.toURI());
+
+List classes = new ArrayList<>();
+
+try (DirectoryStream stream =
Files.newDirectoryStream(classesPath)) {
+for (Path file: stream) {
+Path pathFileName = file.getFileName();
+if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
+String fullClassName = packageName + "." +
+
pathFileName.toString().replace(".class", "");
+
+// Only add classes that can be instantiated via
newInstance()
+try {
+Object obj =
Class.forName(fullClassName).newInstance();
Review comment:
I dont think that is a good idea to instantiate all classes, this makes the
utility rather complicated to use.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ecki commented on a change in pull request #358: ClassUtils.getBaseClasses(desiredBase, packageName)
ecki commented on a change in pull request #358:
ClassUtils.getBaseClasses(desiredBase, packageName)
URL: https://github.com/apache/commons-lang/pull/358#discussion_r244365537
##
File path: src/main/java/org/apache/commons/lang3/ClassUtils.java
##
@@ -1059,6 +1067,85 @@ public static boolean isInnerClass(final Class cls) {
return getClass(loader, className, initialize);
}
+/**
+ * Returns a list of base classes/interfaces underneath the supplied
package.
+ * This method only retrieves base classes/interfaces that have child
classes that can be instantiated
+ * via a no-args constructor.
+ * This only retrieves base classes/interfaces directly underneath the
supplied package.
+ *
+ * @param desiredBase the desired base class/interface to retrieve
+ * @param packageName the package name in the standard import format (i.e.
"java.lang.String")
+ * @param classLoader the class loader to use for retrieving classes
+ * @param The desired base class or interface type to retrieve
+ * @return a list of base classes/interfaces that match the supplied type
underneath the supplied package
+ * @throws IllegalArgumentException if the packageName is invalid
+ * @throws IOException if an I/O error occurs in getting a new directory
stream
+ * @throws NullPointerException if desiredBase, classLoader or url are null
+ * @throws URISyntaxException if the generated url can't be converted to a
URI
+ */
+public static List getBaseClasses(final Class desiredBase, final
String packageName, ClassLoader classLoader)
+throws IllegalArgumentException, IOException,
NullPointerException, URISyntaxException {
+
+Objects.requireNonNull(desiredBase, "desiredBase must not be null");
+
+if (StringUtils.isBlank(packageName)) {
+throw new IllegalArgumentException("packageName must not be
blank");
+}
+
+Objects.requireNonNull(classLoader, "classLoader must not be null");
+
+URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
+Objects.requireNonNull(url, "supplied package not found");
+
+Path classesPath = Paths.get(url.toURI());
Review comment:
Will that work with jmod or jar files or only with class directories?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zmacomber commented on a change in pull request #358: ClassUtils.getBaseClasses(desiredBase, packageName)
zmacomber commented on a change in pull request #358:
ClassUtils.getBaseClasses(desiredBase, packageName)
URL: https://github.com/apache/commons-lang/pull/358#discussion_r244380194
##
File path: src/main/java/org/apache/commons/lang3/ClassUtils.java
##
@@ -1059,6 +1067,62 @@ public static boolean isInnerClass(final Class cls) {
return getClass(loader, className, initialize);
}
+/**
+ * Returns a list of base classes/interfaces underneath the supplied
package
+ * This method only retrieves base classes/interfaces that have children
that can be instantiated
+ * via a no-args constructor
+ * The class loader is retrieved via
Thread.currentThread().getContextClassLoader()
+ * This only retrieves base classes/interfaces directly underneath the
supplied package
+ *
+ * @param desiredBase the desired base class/interface to retrieve
+ * @param packageName the package name in the standard import format (i.e.
"java.lang.String")
+ * @param The desired base class or interface type to retrieve
+ * @return a list of base classes/interfaces that match the supplied type
underneath the supplied package
+ * @throws IllegalArgumentException if the desiredBase or packageName are
invalid
+ * @throws IOException if an I/O error occurs in getting a new directory
stream
+ * @throws NullPointerException if desiredBase or url are null
+ * @throws URISyntaxException if the generated url can't be converted to a
URI
+ */
+public static List getBaseClasses(final Class desiredBase, final
String packageName)
+throws IllegalArgumentException, IOException,
NullPointerException, URISyntaxException {
+
+Objects.requireNonNull(desiredBase, "desiredBase must not be null");
+
+if (StringUtils.isBlank(packageName)) {
+throw new IllegalArgumentException("packageName must not be
blank");
+}
+
+ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
+URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
+Objects.requireNonNull(url, "supplied package not found");
+
+Path classesPath = Paths.get(url.toURI());
+
+List classes = new ArrayList<>();
+
+try (DirectoryStream stream =
Files.newDirectoryStream(classesPath)) {
+for (Path file: stream) {
+Path pathFileName = file.getFileName();
+if (( ! Files.isDirectory(file)) && (pathFileName != null)) {
+String fullClassName = packageName + "." +
+
pathFileName.toString().replace(".class", "");
+
+// Only add classes that can be instantiated via
newInstance()
+try {
+Object obj =
Class.forName(fullClassName).newInstance();
Review comment:
@ecki How is it complicated to use? The unit tests I wrote show it's
simple to use.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zmacomber commented on a change in pull request #358: ClassUtils.getBaseClasses(desiredBase, packageName)
zmacomber commented on a change in pull request #358:
ClassUtils.getBaseClasses(desiredBase, packageName)
URL: https://github.com/apache/commons-lang/pull/358#discussion_r244380396
##
File path: src/main/java/org/apache/commons/lang3/ClassUtils.java
##
@@ -1059,6 +1067,85 @@ public static boolean isInnerClass(final Class cls) {
return getClass(loader, className, initialize);
}
+/**
+ * Returns a list of base classes/interfaces underneath the supplied
package.
+ * This method only retrieves base classes/interfaces that have child
classes that can be instantiated
+ * via a no-args constructor.
+ * This only retrieves base classes/interfaces directly underneath the
supplied package.
+ *
+ * @param desiredBase the desired base class/interface to retrieve
+ * @param packageName the package name in the standard import format (i.e.
"java.lang.String")
+ * @param classLoader the class loader to use for retrieving classes
+ * @param The desired base class or interface type to retrieve
+ * @return a list of base classes/interfaces that match the supplied type
underneath the supplied package
+ * @throws IllegalArgumentException if the packageName is invalid
+ * @throws IOException if an I/O error occurs in getting a new directory
stream
+ * @throws NullPointerException if desiredBase, classLoader or url are null
+ * @throws URISyntaxException if the generated url can't be converted to a
URI
+ */
+public static List getBaseClasses(final Class desiredBase, final
String packageName, ClassLoader classLoader)
+throws IllegalArgumentException, IOException,
NullPointerException, URISyntaxException {
+
+Objects.requireNonNull(desiredBase, "desiredBase must not be null");
+
+if (StringUtils.isBlank(packageName)) {
+throw new IllegalArgumentException("packageName must not be
blank");
+}
+
+Objects.requireNonNull(classLoader, "classLoader must not be null");
+
+URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
+Objects.requireNonNull(url, "supplied package not found");
+
+Path classesPath = Paths.get(url.toURI());
Review comment:
I'm not sure if it will work with jmod or jar files. I designed the method
to just work with class directories.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] zmacomber commented on a change in pull request #358: ClassUtils.getBaseClasses(desiredBase, packageName)
zmacomber commented on a change in pull request #358:
ClassUtils.getBaseClasses(desiredBase, packageName)
URL: https://github.com/apache/commons-lang/pull/358#discussion_r244380396
##
File path: src/main/java/org/apache/commons/lang3/ClassUtils.java
##
@@ -1059,6 +1067,85 @@ public static boolean isInnerClass(final Class cls) {
return getClass(loader, className, initialize);
}
+/**
+ * Returns a list of base classes/interfaces underneath the supplied
package.
+ * This method only retrieves base classes/interfaces that have child
classes that can be instantiated
+ * via a no-args constructor.
+ * This only retrieves base classes/interfaces directly underneath the
supplied package.
+ *
+ * @param desiredBase the desired base class/interface to retrieve
+ * @param packageName the package name in the standard import format (i.e.
"java.lang.String")
+ * @param classLoader the class loader to use for retrieving classes
+ * @param The desired base class or interface type to retrieve
+ * @return a list of base classes/interfaces that match the supplied type
underneath the supplied package
+ * @throws IllegalArgumentException if the packageName is invalid
+ * @throws IOException if an I/O error occurs in getting a new directory
stream
+ * @throws NullPointerException if desiredBase, classLoader or url are null
+ * @throws URISyntaxException if the generated url can't be converted to a
URI
+ */
+public static List getBaseClasses(final Class desiredBase, final
String packageName, ClassLoader classLoader)
+throws IllegalArgumentException, IOException,
NullPointerException, URISyntaxException {
+
+Objects.requireNonNull(desiredBase, "desiredBase must not be null");
+
+if (StringUtils.isBlank(packageName)) {
+throw new IllegalArgumentException("packageName must not be
blank");
+}
+
+Objects.requireNonNull(classLoader, "classLoader must not be null");
+
+URL url = classLoader.getResource(packageName.replaceAll("[.]", "/"));
+Objects.requireNonNull(url, "supplied package not found");
+
+Path classesPath = Paths.get(url.toURI());
Review comment:
@ecki I'm not sure if it will work with jmod or jar files. I designed the
method to just work with class directories.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] Yash-777 commented on a change in pull request #395: Replaces the given String, with the String which is nested in between two Strings.
Yash-777 commented on a change in pull request #395: Replaces the given String,
with the String which is nested in between two Strings.
URL: https://github.com/apache/commons-lang/pull/395#discussion_r247473007
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -3124,6 +3124,41 @@ public static String substringBetween(final String str,
final String open, final
return list.toArray(new String [list.size()]);
}
+/**
+ * Replaces the given String, with the String which is nested in
between two Strings.
+ *
+ * A {@code null} input String returns {@code null}.
+ * A {@code null} open/close returns {@code null} (no match).
+ * An empty ("") open and close returns an empty string.
+ *
+ * @param str the String containing the substring, may be null
+ * @param replace the Sting to be replaced, which is nested in between
open and close substrings
+ * @param open the String before the substring, may be null
+ * @param close the String after the substring, may be null
+ * @return the substring, {@code null} if no match
+ */
+public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
+if (str == null || open == null || close == null) {
+return null;
+}
+final int start = str.indexOf(open);
+if (start != INDEX_NOT_FOUND) {
+String preceding = "";
+if (start > 0) {
+preceding = str.substring(0, start);
Review comment:
Thanks a lot,
As per your suggestion have changed the code as follows.
```java
public static String replaceSubstringInBetween(final String str, final
String replace, final String open, final String close) {
if (str == null) {
return null;
}
if (open == null || close == null) {
return str;
}
final int start = str.indexOf(open);
String preceding = str.substring(0, start + open.length());
final int end = str.indexOf(close, start + open.length());
String exceeding = str.substring(end);
return preceding + open + replace + exceeding;
}
```
Please suggest that the name of the function is valid
`replaceSubstringInBetween()` or do i need to change the name.
I will add more unit test on this function.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] asciborek commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
asciborek commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r243089452
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
So maybe instead of "sz" the name should be "i" or "index"? It's quite good
name for a looping-control variable, isn't?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rosti-il commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
rosti-il commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r243110330
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
Or `idx`? But aren't we spend too much time for a discussion of one variable
name? :-)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] rosti-il commented on issue #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
rosti-il commented on issue #392: LANG-1410: eliminate one unnecessary local int variable and add more tests URL: https://github.com/apache/commons-lang/pull/392#issuecomment-448407328 Force-pushed twice from 27b1742 to 9d8ac06 and then back from 9d8ac06 to 27b1742 because of a confusion about the reason of the Travis CI build failure. The reason of this failure is the a410aab6515931489dc2594d4e0f0ecac2c66071 commit of @garydgregory at master branch. Will the Travis CI build run again automatically after @garydgregory fixed his commit? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one unnecessary local int variable and add more tests
MarkDacek commented on a change in pull request #392: LANG-1410: eliminate one
unnecessary local int variable and add more tests
URL: https://github.com/apache/commons-lang/pull/392#discussion_r243150110
##
File path: src/main/java/org/apache/commons/lang3/StringUtils.java
##
@@ -338,15 +338,17 @@ public static boolean isAllEmpty(final CharSequence...
css) {
* @since 3.0 Changed signature from isBlank(String) to
isBlank(CharSequence)
*/
public static boolean isBlank(final CharSequence cs) {
-int strLen;
Review comment:
Hm, I've given another look through and it would appear that other uses of
`sz` are final as well.
I don't object to this, but I would question the need for it as the memory
gain isn't noticeable.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-rng] asfgit merged pull request #33: RNG-87: Advance MultiplyWithCarry256 state one index per nextInt() call.
asfgit merged pull request #33: RNG-87: Advance MultiplyWithCarry256 state one index per nextInt() call. URL: https://github.com/apache/commons-rng/pull/33 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #28: fixed for AbstractFileObject#toString leak password!
garydgregory commented on issue #28: fixed for AbstractFileObject#toString leak password! URL: https://github.com/apache/commons-vfs/pull/28#issuecomment-478961351 Any thoughts on the likelihood of this breaking existing apps? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x
garydgregory commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x URL: https://github.com/apache/commons-vfs/pull/52#issuecomment-478964021 Hi @woonsan , To be clear, the tests are still failing :-( Gary 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] boris-petrov opened a new pull request #57: Simplify adding files to DefaultFileMonitor
boris-petrov opened a new pull request #57: Simplify adding files to DefaultFileMonitor URL: https://github.com/apache/commons-vfs/pull/57 The original code tried adding recursively the children twice - once in `doAddFile` and then again in `addFile` itself. This is now simplified. Also fix a test which should work only in recursive mode and add another one. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] woonsan commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x
woonsan commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x URL: https://github.com/apache/commons-vfs/pull/52#issuecomment-479276504 @garydgregory , just fyi, I'm trying to figure out how to reproduce this locally. Strangely, my local tests were successful... :( > junit.framework.AssertionFailedError: webdav://admin@localhost:36382/repository/default/read-tests/dir1/subdir4.jar > ... 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] woonsan commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x
woonsan commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x URL: https://github.com/apache/commons-vfs/pull/52#issuecomment-479279513 Hi @garydgregory , I think I found the cause. It's all my bad. For some reason -- probably due to my git env setting, some resource files were not committed/pushed. e.g, commons-vfs2-jackrabbit2/src/test/resources/test-data/read-tests/subdir4.jar and its descendants, commons-vfs2-jackrabbit2/src/test/resources/test-data/*.gz, *.jar, *.tar, ... Sorry and thanks for noticing this problem! I'll fix it asap. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #31: Geometry 29 2 sven
darkma773r commented on a change in pull request #31: Geometry 29 2 sven
URL: https://github.com/apache/commons-geometry/pull/31#discussion_r271548014
##
File path:
commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Plane.java
##
@@ -16,174 +16,193 @@
*/
package org.apache.commons.geometry.euclidean.threed;
+import java.util.Objects;
+
import org.apache.commons.geometry.core.exception.IllegalNormException;
import org.apache.commons.geometry.core.partitioning.Embedding;
import org.apache.commons.geometry.core.partitioning.Hyperplane;
import org.apache.commons.geometry.core.precision.DoublePrecisionContext;
-import org.apache.commons.geometry.euclidean.internal.Vectors;
import org.apache.commons.geometry.euclidean.oned.Vector1D;
import
org.apache.commons.geometry.euclidean.threed.rotation.QuaternionRotation;
import org.apache.commons.geometry.euclidean.twod.PolygonsSet;
import org.apache.commons.geometry.euclidean.twod.Vector2D;
-/** The class represent planes in a three dimensional space.
+/**
+ * The class represents a plane in a three dimensional space.
*/
-public class Plane implements Hyperplane, Embedding {
-
-/** Offset of the origin with respect to the plane. */
-private double originOffset;
+public final class Plane implements Hyperplane, Embedding {
-/** Origin of the plane frame. */
-private Vector3D origin;
+/** First normalized vector of the plane frame (in plane). */
+private final Vector3D u;
-/** First vector of the plane frame (in plane). */
-private Vector3D u;
+/** Second normalized vector of the plane frame (in plane). */
+private final Vector3D v;
-/** Second vector of the plane frame (in plane). */
-private Vector3D v;
+/** Normalized plane normal. */
+private final Vector3D w;
-/** Third vector of the plane frame (plane normal). */
-private Vector3D w;
+/** Offset of the origin with respect to the plane. */
+private final double originOffset;
+/** orthogonal projection of the 3D-space origin in the plane. */
+private final Vector3D projectedOrigin;
+
/** Precision context used to compare floating point numbers. */
private final DoublePrecisionContext precision;
-/** Build a plane normal to a given direction and containing the origin.
- * @param normal normal direction to the plane
+/**
+ * Constructor to build a new plane with the given values.
+ * Made private to prevent inheritance.
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
+ * @param w unit normal vector
+ * @param projectedOrigin orthogonal projection of the 3D-space origin in
the plane.
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @throws IllegalArgumentException if the provided vectors are coplanar
or not normalized
*/
-public Plane(final Vector3D normal, final DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
+private Plane(Vector3D u, Vector3D v, Vector3D w, Vector3D
projectedOrigin, double originOffset,
+DoublePrecisionContext precision) {
+this.u = u;
+this.v = v;
+this.w = w;
+
+if (!areVectorsNormalized(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must be
normalized.");
+}
+if (Vector3D.areCoplanar(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must not be
coplanar.");
+}
+this.projectedOrigin = projectedOrigin;
+this.originOffset = originOffset;
this.precision = precision;
-originOffset = 0;
-setFrame();
}
-
-/** Build a plane from a point and a normal.
- * @param p point belonging to the plane
- * @param normal normal direction to the plane
+
+/**
+ * Build a plane from a point and two (on plane) vectors.
+ * @param p the provided point (on plane)
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @return a new plane
+ * @throws IllegalNormException if the norm of the given values is zero,
NaN, or infinite.
+ * @throws IllegalArgumentException if the provided vectors are collinear
*/
-public Plane(final Vector3D p, final Vector3D normal, final
DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
-this.precision = precision;
-this.originOffset = -p.dot(w);
-setFrame();
+public static Plane fromPointAndPlaneVectors (Vector3D p, final Vector3D
u, final
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #31: Geometry 29 2 sven
darkma773r commented on a change in pull request #31: Geometry 29 2 sven
URL: https://github.com/apache/commons-geometry/pull/31#discussion_r271548380
##
File path:
commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Line.java
##
@@ -236,5 +238,29 @@ public Vector3D intersection(final Line line) {
public SubLine wholeLine() {
return new SubLine(this, new IntervalsSet(precision));
}
+
+
+@Override
+public int hashCode() {
+throw new IllegalStateException("Must not be used in maps.");
Review comment:
We should provide an actual implementation of this method, especially since
`equals()` is overridden.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x
garydgregory commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x URL: https://github.com/apache/commons-vfs/pull/52#issuecomment-479299314 Hi @woonsan, What is the status of this PR? Is it still "Not ready for merge"? Gary 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] woonsan commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x
woonsan commented on issue #52: VFS-686: webdav4 provider based on the latest Jackrabbit 2.x URL: https://github.com/apache/commons-vfs/pull/52#issuecomment-479304944 Hi @garydgregory , Yet not ready status because jackrabbit3 submodule depends on the following (in root pom): > 2.19.2-SNAPSHOT I need to get the release out soon but will take some time. Woonsan 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] woonsan commented on issue #52: [Not to merge - JR snapshot dep] VFS-686: webdav4 provider based on the latest Jackrabbit 2.x
woonsan commented on issue #52: [Not to merge - JR snapshot dep] VFS-686: webdav4 provider based on the latest Jackrabbit 2.x URL: https://github.com/apache/commons-vfs/pull/52#issuecomment-479416774 Hi @garydgregory , I figured out how to make this PR reviewable and mergeable right now: - JCR-4401 is really about simple dependency provision for unit testing in the end. - Therefore, if I add dependencies expanding what I did for JCR-4401 in jackrabbit3 submodule with a custom Main class like what's done for old webdav test case, then this will work, with downgrading the JR dependency to 2.18.0. - I can create a separate JIRA ticket to simplify the test case taking advantage of JCR-4401 later. I'll wrap it up and ask again for reviews soon. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-io] col-panic opened a new pull request #76: Refactor io.monitor to support alternativ file entities
col-panic opened a new pull request #76: Refactor io.monitor to support alternativ file entities URL: https://github.com/apache/commons-io/pull/76 I want to combine the io.monitor with the https://github.com/hierynomus/smbj project, to realize a SMB capable io monitor. In order to achieve this, I have to replace certain hard-coded handles like File with the resp. SMBJ handles. To this end, I started refactoring the code so that I may realize this alternative implementation. Within my IDE i have a prototype of this combination (realized within a separate project) that basically works. My question is: would the commons-io community care for accepting a PR that "opens-up" the monitor for alternative implementations? The PR at hand serves as a basis for discussion. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #31: Geometry 29 2 sven
darkma773r commented on a change in pull request #31: Geometry 29 2 sven
URL: https://github.com/apache/commons-geometry/pull/31#discussion_r268941832
##
File path:
commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Plane.java
##
@@ -16,174 +16,193 @@
*/
package org.apache.commons.geometry.euclidean.threed;
+import java.util.Objects;
+
import org.apache.commons.geometry.core.exception.IllegalNormException;
import org.apache.commons.geometry.core.partitioning.Embedding;
import org.apache.commons.geometry.core.partitioning.Hyperplane;
import org.apache.commons.geometry.core.precision.DoublePrecisionContext;
-import org.apache.commons.geometry.euclidean.internal.Vectors;
import org.apache.commons.geometry.euclidean.oned.Vector1D;
import
org.apache.commons.geometry.euclidean.threed.rotation.QuaternionRotation;
import org.apache.commons.geometry.euclidean.twod.PolygonsSet;
import org.apache.commons.geometry.euclidean.twod.Vector2D;
-/** The class represent planes in a three dimensional space.
+/**
+ * The class represents a plane in a three dimensional space.
*/
-public class Plane implements Hyperplane, Embedding {
-
-/** Offset of the origin with respect to the plane. */
-private double originOffset;
+public final class Plane implements Hyperplane, Embedding {
-/** Origin of the plane frame. */
-private Vector3D origin;
+/** First normalized vector of the plane frame (in plane). */
+private final Vector3D u;
-/** First vector of the plane frame (in plane). */
-private Vector3D u;
+/** Second normalized vector of the plane frame (in plane). */
+private final Vector3D v;
-/** Second vector of the plane frame (in plane). */
-private Vector3D v;
+/** Normalized plane normal. */
+private final Vector3D w;
-/** Third vector of the plane frame (plane normal). */
-private Vector3D w;
+/** Offset of the origin with respect to the plane. */
+private final double originOffset;
+/** orthogonal projection of the 3D-space origin in the plane. */
+private final Vector3D projectedOrigin;
+
/** Precision context used to compare floating point numbers. */
private final DoublePrecisionContext precision;
-/** Build a plane normal to a given direction and containing the origin.
- * @param normal normal direction to the plane
+/**
+ * Constructor to build a new plane with the given values.
+ * Made private to prevent inheritance.
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
+ * @param w unit normal vector
+ * @param projectedOrigin orthogonal projection of the 3D-space origin in
the plane.
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @throws IllegalArgumentException if the provided vectors are coplanar
or not normalized
*/
-public Plane(final Vector3D normal, final DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
+private Plane(Vector3D u, Vector3D v, Vector3D w, Vector3D
projectedOrigin, double originOffset,
+DoublePrecisionContext precision) {
+this.u = u;
+this.v = v;
+this.w = w;
+
+if (!areVectorsNormalized(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must be
normalized.");
+}
+if (Vector3D.areCoplanar(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must not be
coplanar.");
+}
+this.projectedOrigin = projectedOrigin;
+this.originOffset = originOffset;
this.precision = precision;
-originOffset = 0;
-setFrame();
}
-
-/** Build a plane from a point and a normal.
- * @param p point belonging to the plane
- * @param normal normal direction to the plane
+
+/**
+ * Build a plane from a point and two (on plane) vectors.
+ * @param p the provided point (on plane)
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @return a new plane
+ * @throws IllegalNormException if the norm of the given values is zero,
NaN, or infinite.
+ * @throws IllegalArgumentException if the provided vectors are collinear
*/
-public Plane(final Vector3D p, final Vector3D normal, final
DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
-this.precision = precision;
-this.originOffset = -p.dot(w);
-setFrame();
+public static Plane fromPointAndPlaneVectors (Vector3D p, final Vector3D
u, final
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #31: Geometry 29 2 sven
darkma773r commented on a change in pull request #31: Geometry 29 2 sven
URL: https://github.com/apache/commons-geometry/pull/31#discussion_r268946414
##
File path:
commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Plane.java
##
@@ -16,174 +16,193 @@
*/
package org.apache.commons.geometry.euclidean.threed;
+import java.util.Objects;
+
import org.apache.commons.geometry.core.exception.IllegalNormException;
import org.apache.commons.geometry.core.partitioning.Embedding;
import org.apache.commons.geometry.core.partitioning.Hyperplane;
import org.apache.commons.geometry.core.precision.DoublePrecisionContext;
-import org.apache.commons.geometry.euclidean.internal.Vectors;
import org.apache.commons.geometry.euclidean.oned.Vector1D;
import
org.apache.commons.geometry.euclidean.threed.rotation.QuaternionRotation;
import org.apache.commons.geometry.euclidean.twod.PolygonsSet;
import org.apache.commons.geometry.euclidean.twod.Vector2D;
-/** The class represent planes in a three dimensional space.
+/**
+ * The class represents a plane in a three dimensional space.
*/
-public class Plane implements Hyperplane, Embedding {
-
-/** Offset of the origin with respect to the plane. */
-private double originOffset;
+public final class Plane implements Hyperplane, Embedding {
-/** Origin of the plane frame. */
-private Vector3D origin;
+/** First normalized vector of the plane frame (in plane). */
+private final Vector3D u;
-/** First vector of the plane frame (in plane). */
-private Vector3D u;
+/** Second normalized vector of the plane frame (in plane). */
+private final Vector3D v;
-/** Second vector of the plane frame (in plane). */
-private Vector3D v;
+/** Normalized plane normal. */
+private final Vector3D w;
-/** Third vector of the plane frame (plane normal). */
-private Vector3D w;
+/** Offset of the origin with respect to the plane. */
+private final double originOffset;
+/** orthogonal projection of the 3D-space origin in the plane. */
+private final Vector3D projectedOrigin;
+
/** Precision context used to compare floating point numbers. */
private final DoublePrecisionContext precision;
-/** Build a plane normal to a given direction and containing the origin.
- * @param normal normal direction to the plane
+/**
+ * Constructor to build a new plane with the given values.
+ * Made private to prevent inheritance.
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
+ * @param w unit normal vector
+ * @param projectedOrigin orthogonal projection of the 3D-space origin in
the plane.
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @throws IllegalArgumentException if the provided vectors are coplanar
or not normalized
*/
-public Plane(final Vector3D normal, final DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
+private Plane(Vector3D u, Vector3D v, Vector3D w, Vector3D
projectedOrigin, double originOffset,
+DoublePrecisionContext precision) {
+this.u = u;
+this.v = v;
+this.w = w;
+
+if (!areVectorsNormalized(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must be
normalized.");
+}
+if (Vector3D.areCoplanar(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must not be
coplanar.");
+}
+this.projectedOrigin = projectedOrigin;
+this.originOffset = originOffset;
this.precision = precision;
-originOffset = 0;
-setFrame();
}
-
-/** Build a plane from a point and a normal.
- * @param p point belonging to the plane
- * @param normal normal direction to the plane
+
+/**
+ * Build a plane from a point and two (on plane) vectors.
+ * @param p the provided point (on plane)
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @return a new plane
+ * @throws IllegalNormException if the norm of the given values is zero,
NaN, or infinite.
+ * @throws IllegalArgumentException if the provided vectors are collinear
*/
-public Plane(final Vector3D p, final Vector3D normal, final
DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
-this.precision = precision;
-this.originOffset = -p.dot(w);
-setFrame();
+public static Plane fromPointAndPlaneVectors (Vector3D p, final Vector3D
u, final
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #31: Geometry 29 2 sven
darkma773r commented on a change in pull request #31: Geometry 29 2 sven
URL: https://github.com/apache/commons-geometry/pull/31#discussion_r268943199
##
File path:
commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Plane.java
##
@@ -16,174 +16,193 @@
*/
package org.apache.commons.geometry.euclidean.threed;
+import java.util.Objects;
+
import org.apache.commons.geometry.core.exception.IllegalNormException;
import org.apache.commons.geometry.core.partitioning.Embedding;
import org.apache.commons.geometry.core.partitioning.Hyperplane;
import org.apache.commons.geometry.core.precision.DoublePrecisionContext;
-import org.apache.commons.geometry.euclidean.internal.Vectors;
import org.apache.commons.geometry.euclidean.oned.Vector1D;
import
org.apache.commons.geometry.euclidean.threed.rotation.QuaternionRotation;
import org.apache.commons.geometry.euclidean.twod.PolygonsSet;
import org.apache.commons.geometry.euclidean.twod.Vector2D;
-/** The class represent planes in a three dimensional space.
+/**
+ * The class represents a plane in a three dimensional space.
*/
-public class Plane implements Hyperplane, Embedding {
-
-/** Offset of the origin with respect to the plane. */
-private double originOffset;
+public final class Plane implements Hyperplane, Embedding {
-/** Origin of the plane frame. */
-private Vector3D origin;
+/** First normalized vector of the plane frame (in plane). */
+private final Vector3D u;
-/** First vector of the plane frame (in plane). */
-private Vector3D u;
+/** Second normalized vector of the plane frame (in plane). */
+private final Vector3D v;
-/** Second vector of the plane frame (in plane). */
-private Vector3D v;
+/** Normalized plane normal. */
+private final Vector3D w;
-/** Third vector of the plane frame (plane normal). */
-private Vector3D w;
+/** Offset of the origin with respect to the plane. */
+private final double originOffset;
+/** orthogonal projection of the 3D-space origin in the plane. */
+private final Vector3D projectedOrigin;
+
/** Precision context used to compare floating point numbers. */
private final DoublePrecisionContext precision;
-/** Build a plane normal to a given direction and containing the origin.
- * @param normal normal direction to the plane
+/**
+ * Constructor to build a new plane with the given values.
+ * Made private to prevent inheritance.
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
+ * @param w unit normal vector
+ * @param projectedOrigin orthogonal projection of the 3D-space origin in
the plane.
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @throws IllegalArgumentException if the provided vectors are coplanar
or not normalized
*/
-public Plane(final Vector3D normal, final DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
+private Plane(Vector3D u, Vector3D v, Vector3D w, Vector3D
projectedOrigin, double originOffset,
+DoublePrecisionContext precision) {
+this.u = u;
+this.v = v;
+this.w = w;
+
+if (!areVectorsNormalized(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must be
normalized.");
+}
+if (Vector3D.areCoplanar(u, v, w, precision))
+{
+throw new IllegalArgumentException("Provided vectors must not be
coplanar.");
+}
+this.projectedOrigin = projectedOrigin;
+this.originOffset = originOffset;
this.precision = precision;
-originOffset = 0;
-setFrame();
}
-
-/** Build a plane from a point and a normal.
- * @param p point belonging to the plane
- * @param normal normal direction to the plane
+
+/**
+ * Build a plane from a point and two (on plane) vectors.
+ * @param p the provided point (on plane)
+ * @param u u vector (on plane)
+ * @param v v vector (on plane)
* @param precision precision context used to compare floating point values
- * @exception IllegalArgumentException if the normal norm is too small
+ * @return a new plane
+ * @throws IllegalNormException if the norm of the given values is zero,
NaN, or infinite.
+ * @throws IllegalArgumentException if the provided vectors are collinear
*/
-public Plane(final Vector3D p, final Vector3D normal, final
DoublePrecisionContext precision)
-throws IllegalArgumentException {
-setNormal(normal);
-this.precision = precision;
-this.originOffset = -p.dot(w);
-setFrame();
+public static Plane fromPointAndPlaneVectors (Vector3D p, final Vector3D
u, final
[GitHub] [commons-geometry] darkma773r commented on a change in pull request #31: Geometry 29 2 sven
darkma773r commented on a change in pull request #31: Geometry 29 2 sven
URL: https://github.com/apache/commons-geometry/pull/31#discussion_r268945594
##
File path:
commons-geometry-euclidean/src/main/java/org/apache/commons/geometry/euclidean/threed/Plane.java
##
@@ -16,174 +16,193 @@
*/
package org.apache.commons.geometry.euclidean.threed;
+import java.util.Objects;
+
import org.apache.commons.geometry.core.exception.IllegalNormException;
import org.apache.commons.geometry.core.partitioning.Embedding;
import org.apache.commons.geometry.core.partitioning.Hyperplane;
import org.apache.commons.geometry.core.precision.DoublePrecisionContext;
-import org.apache.commons.geometry.euclidean.internal.Vectors;
import org.apache.commons.geometry.euclidean.oned.Vector1D;
import
org.apache.commons.geometry.euclidean.threed.rotation.QuaternionRotation;
import org.apache.commons.geometry.euclidean.twod.PolygonsSet;
import org.apache.commons.geometry.euclidean.twod.Vector2D;
-/** The class represent planes in a three dimensional space.
+/**
+ * The class represents a plane in a three dimensional space.
*/
-public class Plane implements Hyperplane, Embedding {
-
-/** Offset of the origin with respect to the plane. */
-private double originOffset;
+public final class Plane implements Hyperplane, Embedding {
-/** Origin of the plane frame. */
-private Vector3D origin;
+/** First normalized vector of the plane frame (in plane). */
+private final Vector3D u;
-/** First vector of the plane frame (in plane). */
-private Vector3D u;
+/** Second normalized vector of the plane frame (in plane). */
+private final Vector3D v;
-/** Second vector of the plane frame (in plane). */
-private Vector3D v;
+/** Normalized plane normal. */
+private final Vector3D w;
-/** Third vector of the plane frame (plane normal). */
-private Vector3D w;
+/** Offset of the origin with respect to the plane. */
+private final double originOffset;
+/** orthogonal projection of the 3D-space origin in the plane. */
+private final Vector3D projectedOrigin;
Review comment:
Do we need to store this value? It is easily derived from `w` and
`originOffset` and is not used in any internal calculations, so I am leaning
toward calculating it directly in the `getOrigin()` method.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
