[GitHub] ant pull request #78: A new CharSet type to hold available Charset names

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #78: A new CharSet type to hold available Charset names

twogee
GitHub user twogee opened a pull request:

    https://github.com/apache/ant/pull/78

    A new CharSet type to hold available Charset names

    I believe that might be useful when validating "encoding" (or "charset") attributes

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/twogee/ant charset-type

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/ant/pull/78.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 #78
   
----
commit 033d68c7f6ecf382f84eec5ab0c2bb91eb9bd6fb
Author: twogee <g.grigelionis@...>
Date:   2018-11-06T21:27:55Z

    A new CharSet type to hold available Charset names

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/78
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Windows/90/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/78
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Linux/84/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/78
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Windows/92/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/78
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Linux/86/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user bodewig commented on a diff in the pull request:

    https://github.com/apache/ant/pull/78#discussion_r232673446
 
    --- Diff: src/tests/junit/org/apache/tools/ant/types/CharSetTest.java ---
    @@ -0,0 +1,20 @@
    +package org.apache.tools.ant.types;
    +
    +import org.apache.tools.ant.BuildException;
    +import org.junit.Test;
    +
    +import java.util.Arrays;
    +
    +public class CharSetTest {
    +    @Test
    +    public void testCorrectNames() {
    +        String[] expected = {"UTF-8", "ISO-8859-1", "037", "us", "IBM500"};
    +        Arrays.stream(expected).forEach(new CharSet()::setValue);
    +    }
    +
    +    @Test(expected = BuildException.class)
    +    public void testNonExistentNames() {
    +        String[] nonexistent = {"mojibake", "dummy"};
    +        Arrays.stream(nonexistent).forEach(new CharSet()::setValue);
    --- End diff --
   
    is the test for "dummy" ever going to be run?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user bodewig commented on the issue:

    https://github.com/apache/ant/pull/78
 
    Both files need license headers. Other than that, looks good to me.
   
    You envision this type to be used as argument type in `setEncoding` overloads?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user twogee commented on a diff in the pull request:

    https://github.com/apache/ant/pull/78#discussion_r232795363
 
    --- Diff: src/tests/junit/org/apache/tools/ant/types/CharSetTest.java ---
    @@ -0,0 +1,20 @@
    +package org.apache.tools.ant.types;
    +
    +import org.apache.tools.ant.BuildException;
    +import org.junit.Test;
    +
    +import java.util.Arrays;
    +
    +public class CharSetTest {
    +    @Test
    +    public void testCorrectNames() {
    +        String[] expected = {"UTF-8", "ISO-8859-1", "037", "us", "IBM500"};
    +        Arrays.stream(expected).forEach(new CharSet()::setValue);
    +    }
    +
    +    @Test(expected = BuildException.class)
    +    public void testNonExistentNames() {
    +        String[] nonexistent = {"mojibake", "dummy"};
    +        Arrays.stream(nonexistent).forEach(new CharSet()::setValue);
    --- End diff --
   
    I was lazy... 😁 the tests are properly parameterised now


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user twogee commented on the issue:

    https://github.com/apache/ant/pull/78
 
    License headers added. Yes, it should be used with `setEncoding` (or in some tasks `setCharset`). Now that I think about it image/imageio tasks have an "encoding" attribute which is a misnomer. I'd like to deprecate it in imageio and use a "format" instead (with a proper Enumerated Attribute to boot 😁).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user asfgit commented on the issue:

    https://github.com/apache/ant/pull/78
 
   
    Refer to this link for build results (access rights to CI server needed):
    https://builds.apache.org/job/Ant%20Github-PR-Windows/95/



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant issue #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user bodewig commented on the issue:

    https://github.com/apache/ant/pull/78
 
    looks good.
   
    The imageio tasks haven't been part of any release, yet, no reason to deprecate methods, just change them before we cut the next release IMHO.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant pull request #78: A new CharSet type to hold available Charset names

twogee
In reply to this post by twogee
Github user twogee closed the pull request at:

    https://github.com/apache/ant/pull/78


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]