You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by TheRealHaui <gi...@git.apache.org> on 2017/07/17 23:27:55 UTC
[GitHub] commons-text pull request #56: add-some-Unit Tests
GitHub user TheRealHaui opened a pull request:
https://github.com/apache/commons-text/pull/56
add-some-Unit Tests
I have created new Unit Tests which cover previously untested lines of code which I want to contribute.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/TheRealHaui/commons-text add-some-Unit-Tests
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/commons-text/pull/56.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #56
----
commit acad335ca6c2c08644063ab0dc0be1e8233d430e
Author: Michael Hausegger <ha...@googlemail.com>
Date: 2017-07-17T23:26:06Z
add-some-Unit Tests Created Unit Tests to increase code coverage.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on the issue:
https://github.com/apache/commons-text/pull/56
Oh sorry @TheRealHaui, I just reread my main comment in my review from a few minutes ago, and I realise that I may have sounded confrontational. I had not meant to come across as such, so I wish to apologise if it sounded that way. 🙇
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854299
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -55,11 +55,11 @@ public void testExtendedFormats() {
final String pattern = "Lower: {0,lower} Upper: {1,upper}";
final ExtendedMessageFormat emf = new ExtendedMessageFormat(pattern, registry);
assertEquals("TOPATTERN", pattern, emf.toPattern());
- assertEquals("Lower: foo Upper: BAR", emf.format(new Object[] {"foo", "bar"}));
- assertEquals("Lower: foo Upper: BAR", emf.format(new Object[] {"Foo", "Bar"}));
- assertEquals("Lower: foo Upper: BAR", emf.format(new Object[] {"FOO", "BAR"}));
- assertEquals("Lower: foo Upper: BAR", emf.format(new Object[] {"FOO", "bar"}));
- assertEquals("Lower: foo Upper: BAR", emf.format(new Object[] {"foo", "BAR"}));
+ assertEquals("Lower: foo Upper: BAR", emf.format(new Object[]{"foo", "bar"}));
+ assertEquals("Lower: foo Upper: BAR", emf.format(new Object[]{"Foo", "Bar"}));
+ assertEquals("Lower: foo Upper: BAR", emf.format(new Object[]{"FOO", "BAR"}));
+ assertEquals("Lower: foo Upper: BAR", emf.format(new Object[]{"FOO", "bar"}));
+ assertEquals("Lower: foo Upper: BAR", emf.format(new Object[]{"foo", "BAR"}));
--- End diff --
Unless whitespace changes like this one are required by Checkstyle, please consider reverting them to prevent polluting the Git history.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854015
--- Diff: src/test/java/org/apache/commons/text/AlphabetConverterTest.java ---
@@ -201,4 +207,97 @@ private void test(final Character[] originalChars, final Character[] encodingCha
Assert.assertEquals("Encoded '" + s + "' into '" + encoded + "', but decoded into '" + decoded + "'", s, decoded);
}
}
+
+ @Test
+ public void testCreateConverterFromCharsWithNullAndNull() {
+
--- End diff --
Blank lines at the start of methods, like this one, seem inconsistent with the rest of the code base.
Please consider removing them from _all_ methods which you've created and touched.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by TheRealHaui <gi...@git.apache.org>.
Github user TheRealHaui commented on the issue:
https://github.com/apache/commons-text/pull/56
You mean the cases reported by the checkstyle plugin?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-text/pull/56
[](https://coveralls.io/builds/12526449)
Coverage increased (+0.9%) to 98.163% when pulling **12129b2beafc74f8f04957cc52b464a3eacdd3a9 on TheRealHaui:add-some-Unit-Tests** into **aaf4aba369ed0b97d17bc9343f763b0d099dbc2f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on the issue:
https://github.com/apache/commons-text/pull/56
Thanks! :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by TheRealHaui <gi...@git.apache.org>.
Github user TheRealHaui commented on the issue:
https://github.com/apache/commons-text/pull/56
You're welcome.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by TheRealHaui <gi...@git.apache.org>.
Github user TheRealHaui commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r129109286
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -416,6 +416,81 @@ public void testSetFormatsByArgumentIndex() {
emf.setFormatsByArgumentIndex(new Format[]{new LowerCaseFormat(), new UpperCaseFormat()});
}
+ @Test
+ public void testFailsToCreateExtendedMessageFormatTakingThreeArgumentsThrowsIllegalArgumentExceptionOne() {
+ Map<String, FormatFactory> map = new HashMap<String, FormatFactory>();
+ ExtendedMessageFormat extendedMessageFormat = null;
--- End diff --
Corrected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854888
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -338,10 +339,11 @@ private void checkBuiltInFormat(final String pattern, final Object[] args, final
/**
* Test a built in format for the specified Locales, plus <code>null</code> Locale.
- * @param pattern MessageFormat pattern
+ *
+ * @param pattern MessageFormat pattern
* @param fmtRegistry FormatFactory registry to use
- * @param args MessageFormat arguments
- * @param locales to test
+ * @param args MessageFormat arguments
+ * @param locales to test
--- End diff --
I think the lines were fine before, please consider reverting them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on the issue:
https://github.com/apache/commons-text/pull/56
@ameyjadiye Try adding the same suppression as commons-lang:
```
<suppress checks="JavadocMethod" files=".*[/\\]test[/\\].*"/>
<suppress checks="JavadocPackage" files=".*[/\\]test[/\\].*"/>
```
That should reduce the errors. Maybe their are other checks which can be reasonably suppressed for tests?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r128595088
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -416,6 +416,81 @@ public void testSetFormatsByArgumentIndex() {
emf.setFormatsByArgumentIndex(new Format[]{new LowerCaseFormat(), new UpperCaseFormat()});
}
+ @Test
+ public void testFailsToCreateExtendedMessageFormatTakingThreeArgumentsThrowsIllegalArgumentExceptionOne() {
+ Map<String, FormatFactory> map = new HashMap<String, FormatFactory>();
--- End diff --
As commons-text requires Java 7 this can be reduced to `Map<String, FormatFactory> map = new HashMap<>();`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-text/pull/56
[](https://coveralls.io/builds/12445709)
Coverage increased (+1.04%) to 98.34% when pulling **04788b92e136e4e621c6b2799e2d86d52cd8f7d5 on TheRealHaui:add-some-Unit-Tests** into **aaf4aba369ed0b97d17bc9343f763b0d099dbc2f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r128594766
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -416,6 +416,81 @@ public void testSetFormatsByArgumentIndex() {
emf.setFormatsByArgumentIndex(new Format[]{new LowerCaseFormat(), new UpperCaseFormat()});
}
+ @Test
+ public void testFailsToCreateExtendedMessageFormatTakingThreeArgumentsThrowsIllegalArgumentExceptionOne() {
--- End diff --
The name of this test method (and some of the ones below) seems misleading, because the name suggests that the three argument constructor is tested, but the tests calls the two argument constructor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127855372
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -426,10 +517,13 @@ public void testSetFormatsByArgumentIndex() {
@Override
public StringBuffer format(final Object obj, final StringBuffer toAppendTo, final FieldPosition pos) {
- return toAppendTo.append(((String)obj).toLowerCase());
+ return toAppendTo.append(((String) obj).toLowerCase());
--- End diff --
Whitespace changes like this are _good_ IMO, as they improve code clarity. Well done! 👍
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127855540
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -476,8 +578,8 @@ public Format getFormat(final String name, final String arguments, final Locale
public Format getFormat(final String name, final String arguments, final Locale locale) {
return !"short".equals(arguments) ? null
: locale == null ? DateFormat
- .getDateInstance(DateFormat.DEFAULT) : DateFormat
- .getDateInstance(DateFormat.DEFAULT, locale);
+ .getDateInstance(DateFormat.DEFAULT) : DateFormat
+ .getDateInstance(DateFormat.DEFAULT, locale);
--- End diff --
I think these lines were reasonably indented before. Unless Checkstyle complains, please consider reverting these lines.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854991
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -353,10 +355,11 @@ private void checkBuiltInFormat(final String pattern, final Map<String, ?> fmtRe
/**
* Create an ExtendedMessageFormat for the specified pattern and locale and check the
* formated output matches the expected result for the parameters.
- * @param pattern string
+ *
+ * @param pattern string
* @param registryUnused map (currently unused)
- * @param args Object[]
- * @param locale Locale
+ * @param args Object[]
+ * @param locale Locale
--- End diff --
Ditto.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127855769
--- Diff: src/test/java/org/apache/commons/text/StrSubstitutorTest.java ---
@@ -17,21 +17,16 @@
package org.apache.commons.text;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertSame;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import org.apache.commons.lang3.mutable.MutableObject;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
import java.util.HashMap;
import java.util.Map;
import java.util.Properties;
-import org.apache.commons.lang3.mutable.MutableObject;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
+import static org.junit.Assert.*;
--- End diff --
I think the use of non-wildcard `Assert.assert*` imports was fine before. Please consider expanding these imports back into proper ones.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by TheRealHaui <gi...@git.apache.org>.
Github user TheRealHaui commented on the issue:
https://github.com/apache/commons-text/pull/56
Obviously a problem occurred with Travis:
> The command "eval mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V " failed. Retrying, 2 of 3.
> Error: JAVA_HOME is not defined correctly.
> We cannot execute /usr/lib/jvm/java-7-oracle/bin/java
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854120
--- Diff: src/test/java/org/apache/commons/text/AlphabetConverterTest.java ---
@@ -201,4 +207,97 @@ private void test(final Character[] originalChars, final Character[] encodingCha
Assert.assertEquals("Encoded '" + s + "' into '" + encoded + "', but decoded into '" + decoded + "'", s, decoded);
}
}
+
+ @Test
+ public void testCreateConverterFromCharsWithNullAndNull() {
+
+ Character[] characterArray = new Character[2];
+ characterArray[0] = Character.valueOf('$');
+ characterArray[1] = characterArray[0];
+
+ try {
+ AlphabetConverter.createConverterFromChars(characterArray, null, null);
+ fail("Expecting exception: IllegalArgumentException");
+ } catch (IllegalArgumentException e) {
+ assertEquals(AlphabetConverter.class.getName(), e.getStackTrace()[0].getClassName());
+ }
+
--- End diff --
Blank lines at the end of methods, like this one, seem inconsistent with the rest of the code base.
Please consider removing them from _all_ methods which you've created and touched.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by TheRealHaui <gi...@git.apache.org>.
Github user TheRealHaui commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r129108740
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -416,6 +416,81 @@ public void testSetFormatsByArgumentIndex() {
emf.setFormatsByArgumentIndex(new Format[]{new LowerCaseFormat(), new UpperCaseFormat()});
}
+ @Test
+ public void testFailsToCreateExtendedMessageFormatTakingThreeArgumentsThrowsIllegalArgumentExceptionOne() {
+ Map<String, FormatFactory> map = new HashMap<String, FormatFactory>();
--- End diff --
Corrected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854963
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -353,10 +355,11 @@ private void checkBuiltInFormat(final String pattern, final Map<String, ?> fmtRe
/**
* Create an ExtendedMessageFormat for the specified pattern and locale and check the
* formated output matches the expected result for the parameters.
- * @param pattern string
+ *
+ * @param pattern string
--- End diff --
Ditto.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by ameyjadiye <gi...@git.apache.org>.
Github user ameyjadiye commented on the issue:
https://github.com/apache/commons-text/pull/56
@PascalSchumacher I just enabled in my local, seems very cumbersome task to me at this point.
```
<snip>
[INFO] There are 2551 checkstyle errors.
.
.
.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 41.252 s
[INFO] Finished at: 2017-07-18T23:18:57+05:30
[INFO] Final Memory: 36M/621M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.13:check (default-cli) on project commons-text: You have 2551 Checkstyle violations. -> [Help 1]
.
.
.
</snip>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by TheRealHaui <gi...@git.apache.org>.
Github user TheRealHaui commented on the issue:
https://github.com/apache/commons-text/pull/56
Reformatted requested code artifacts.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127856024
--- Diff: src/test/java/org/apache/commons/text/similarity/LevenshteinResultsTest.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.text.similarity;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * Unit tests for class {@link LevenshteinResults}.
+ *
+ * @date 2017-07-17
+ * @see LevenshteinResults
+ **/
--- End diff --
I struggle to see how this Javadoc is useful. Please consider removing it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854930
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -338,10 +339,11 @@ private void checkBuiltInFormat(final String pattern, final Object[] args, final
/**
* Test a built in format for the specified Locales, plus <code>null</code> Locale.
- * @param pattern MessageFormat pattern
+ *
+ * @param pattern MessageFormat pattern
--- End diff --
I think this line was fine before, please consider reverting it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by TheRealHaui <gi...@git.apache.org>.
Github user TheRealHaui commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r129106989
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -416,6 +416,81 @@ public void testSetFormatsByArgumentIndex() {
emf.setFormatsByArgumentIndex(new Format[]{new LowerCaseFormat(), new UpperCaseFormat()});
}
+ @Test
+ public void testFailsToCreateExtendedMessageFormatTakingThreeArgumentsThrowsIllegalArgumentExceptionOne() {
--- End diff --
Corrected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on the issue:
https://github.com/apache/commons-text/pull/56
Thanks @TheRealHaui!
I think handling the rest of the cases reported by Checkstyle should be done in a separate PR (or series of PRs if needed) by whoever feels up to it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-text/pull/56
[](https://coveralls.io/builds/12526449)
Coverage increased (+0.9%) to 98.163% when pulling **12129b2beafc74f8f04957cc52b464a3eacdd3a9 on TheRealHaui:add-some-Unit-Tests** into **aaf4aba369ed0b97d17bc9343f763b0d099dbc2f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/commons-text/pull/56
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127854827
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -295,41 +295,42 @@ public void testEqualsHashcode() {
ExtendedMessageFormat other = null;
// Same object
- assertTrue("same, equals()", emf.equals(emf));
+ assertTrue("same, equals()", emf.equals(emf));
assertTrue("same, hashcode()", emf.hashCode() == emf.hashCode());
assertFalse("null, equals", emf.equals(null));
// Equal Object
other = new ExtendedMessageFormat(pattern, Locale.US, fmtRegistry);
- assertTrue("equal, equals()", emf.equals(other));
+ assertTrue("equal, equals()", emf.equals(other));
assertTrue("equal, hashcode()", emf.hashCode() == other.hashCode());
// Different Class
other = new OtherExtendedMessageFormat(pattern, Locale.US, fmtRegistry);
- assertFalse("class, equals()", emf.equals(other));
+ assertFalse("class, equals()", emf.equals(other));
assertTrue("class, hashcode()", emf.hashCode() == other.hashCode()); // same hashcode
// Different pattern
other = new ExtendedMessageFormat("X" + pattern, Locale.US, fmtRegistry);
- assertFalse("pattern, equals()", emf.equals(other));
+ assertFalse("pattern, equals()", emf.equals(other));
assertFalse("pattern, hashcode()", emf.hashCode() == other.hashCode());
// Different registry
other = new ExtendedMessageFormat(pattern, Locale.US, otherRegitry);
- assertFalse("registry, equals()", emf.equals(other));
+ assertFalse("registry, equals()", emf.equals(other));
assertFalse("registry, hashcode()", emf.hashCode() == other.hashCode());
// Different Locale
other = new ExtendedMessageFormat(pattern, Locale.FRANCE, fmtRegistry);
- assertFalse("locale, equals()", emf.equals(other));
+ assertFalse("locale, equals()", emf.equals(other));
assertTrue("locale, hashcode()", emf.hashCode() == other.hashCode()); // same hashcode
}
/**
* Test a built in format for the specified Locales, plus <code>null</code> Locale.
+ *
* @param pattern MessageFormat pattern
- * @param args MessageFormat arguments
+ * @param args MessageFormat arguments
--- End diff --
I think this line was fine before. Please consider reverting it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on the issue:
https://github.com/apache/commons-text/pull/56
@jbduncan Check-style for commons-text does is currently not applied to tests. I think we should look into enabling it.
@TheRealHaui Please fix the things @jbduncan suggested. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on the issue:
https://github.com/apache/commons-text/pull/56
@TheRealHaui Yes, precisely. :)
(With hindsight, I should have used the word "violations" instead of "cases"; it would have been clearer that I was referring to the errors that Checkstyle reports, rather than something else.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text issue #56: add-some-Unit Tests
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-text/pull/56
[](https://coveralls.io/builds/12432497)
Coverage increased (+1.04%) to 98.34% when pulling **91039cb23f79921238afebbb4bff3ccbbd049e6a on TheRealHaui:add-some-Unit-Tests** into **aaf4aba369ed0b97d17bc9343f763b0d099dbc2f on apache:master**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127855137
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -373,14 +376,15 @@ private void checkBuiltInFormat(final String pattern, final Map<String, ?> regis
} else {
emf = new ExtendedMessageFormat(pattern, locale);
}
- assertEquals("format " + buffer.toString(), mf.format(args), emf.format(args));
+ assertEquals("format " + buffer.toString(), mf.format(args), emf.format(args));
assertEquals("toPattern " + buffer.toString(), mf.toPattern(), emf.toPattern());
}
/**
* Replace MessageFormat(String, Locale) constructor (not available until JDK 1.4).
+ *
* @param pattern string
- * @param locale Locale
+ * @param locale Locale
--- End diff --
Ditto.
Please consider reverting whitespace changes for _all_ `@param` lines where you only inserted whitespaces between the parameter name and its description.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by PascalSchumacher <gi...@git.apache.org>.
Github user PascalSchumacher commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r128595517
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -416,6 +416,81 @@ public void testSetFormatsByArgumentIndex() {
emf.setFormatsByArgumentIndex(new Format[]{new LowerCaseFormat(), new UpperCaseFormat()});
}
+ @Test
+ public void testFailsToCreateExtendedMessageFormatTakingThreeArgumentsThrowsIllegalArgumentExceptionOne() {
+ Map<String, FormatFactory> map = new HashMap<String, FormatFactory>();
+ ExtendedMessageFormat extendedMessageFormat = null;
--- End diff --
No need to create a variable if it is not used. This will produce unused variable warnings in IDEs etc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
[GitHub] commons-text pull request #56: add-some-Unit Tests
Posted by jbduncan <gi...@git.apache.org>.
Github user jbduncan commented on a diff in the pull request:
https://github.com/apache/commons-text/pull/56#discussion_r127855604
--- Diff: src/test/java/org/apache/commons/text/ExtendedMessageFormatTest.java ---
@@ -488,10 +590,10 @@ public Format getFormat(final String name, final String arguments, final Locale
private static final long serialVersionUID = 1L;
public OtherExtendedMessageFormat(final String pattern, final Locale locale,
- final Map<String, ? extends FormatFactory> registry) {
+ final Map<String, ? extends FormatFactory> registry) {
--- End diff --
I think these lines were reasonably indented before. Unless Checkstyle complains, please consider reverting these lines.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org