You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/06/09 18:54:15 UTC

[GitHub] [commons-io] LinuxClient opened a new pull request #120: Add caching for required charsets

LinuxClient opened a new pull request #120:
URL: https://github.com/apache/commons-io/pull/120


   I have seen the following comment and thought to do this:
   `// maybe cache?`


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on pull request #120:
URL: https://github.com/apache/commons-io/pull/120#issuecomment-641637397


   I don't know if I keep overlooking things, but I can't find the errors that Travis CI says. 


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438149732



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -72,15 +83,7 @@
      * @since 2.5
      */
     public static SortedMap<String, Charset> requiredCharsets() {

Review comment:
       Because `Collections.unmodifiableSortedMap(REQUIRED_CHARSETS);` returns a SortedMap. We could use `Collections.unmodifiableMap(REQUIRED_CHARSETS);` or `new TreeMap(REQUIRED_CHARSETS);` to change the return type.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] garydgregory merged pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #120:
URL: https://github.com/apache/commons-io/pull/120


   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient removed a comment on pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient removed a comment on pull request #120:
URL: https://github.com/apache/commons-io/pull/120#issuecomment-641637397


   I don't know if I keep overlooking things, but I can't find the errors that Travis CI says. 


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438360990



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -71,16 +83,8 @@
      * @see Charset#availableCharsets()
      * @since 2.5
      */
-    public static SortedMap<String, Charset> requiredCharsets() {
-        // maybe cache?
-        final TreeMap<String, Charset> m = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
-        m.put(StandardCharsets.ISO_8859_1.name(), StandardCharsets.ISO_8859_1);
-        m.put(StandardCharsets.US_ASCII.name(), StandardCharsets.US_ASCII);
-        m.put(StandardCharsets.UTF_16.name(), StandardCharsets.UTF_16);
-        m.put(StandardCharsets.UTF_16BE.name(), StandardCharsets.UTF_16BE);
-        m.put(StandardCharsets.UTF_16LE.name(), StandardCharsets.UTF_16LE);
-        m.put(StandardCharsets.UTF_8.name(), StandardCharsets.UTF_8);
-        return Collections.unmodifiableSortedMap(m);
+    public static Map<String, Charset> requiredCharsets() {
+        return Collections.unmodifiableSortedMap(REQUIRED_CHARSETS);

Review comment:
       Is there a reason why `REQUIRED_CHARSETS` is modifiable but not the value returned by this 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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient removed a comment on pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient removed a comment on pull request #120:
URL: https://github.com/apache/commons-io/pull/120#issuecomment-641588444


   I excuse me if this PR is a bit large. I've omitted to create a separate PR.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on pull request #120:
URL: https://github.com/apache/commons-io/pull/120#issuecomment-641588444


   I excuse me if this PR is a bit large. I've omitted to create a separate PR.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438293467



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -72,15 +83,7 @@
      * @since 2.5
      */
     public static SortedMap<String, Charset> requiredCharsets() {

Review comment:
       I have now changed it. Could you look over it again? 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438400351



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -71,16 +85,8 @@
      * @see Charset#availableCharsets()
      * @since 2.5
      */
-    public static SortedMap<String, Charset> requiredCharsets() {
-        // maybe cache?
-        final TreeMap<String, Charset> m = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
-        m.put(StandardCharsets.ISO_8859_1.name(), StandardCharsets.ISO_8859_1);
-        m.put(StandardCharsets.US_ASCII.name(), StandardCharsets.US_ASCII);
-        m.put(StandardCharsets.UTF_16.name(), StandardCharsets.UTF_16);
-        m.put(StandardCharsets.UTF_16BE.name(), StandardCharsets.UTF_16BE);
-        m.put(StandardCharsets.UTF_16LE.name(), StandardCharsets.UTF_16LE);
-        m.put(StandardCharsets.UTF_8.name(), StandardCharsets.UTF_8);
-        return Collections.unmodifiableSortedMap(m);
+    public static Map<String, Charset> requiredCharsets() {

Review comment:
       @LinuxClient 
   You cannot change the return type in this manner as it breaks binary compatibility,




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] garydgregory commented on pull request #120: Add caching for required charsets and cleanup code

Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #120:
URL: https://github.com/apache/commons-io/pull/120#issuecomment-641909869


   There are too many unrelated changes here as you are changing 19 filles. Please focus the PR on what you really want to do.


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] coveralls commented on pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #120:
URL: https://github.com/apache/commons-io/pull/120#issuecomment-642062946


   
   [![Coverage Status](https://coveralls.io/builds/31366647/badge)](https://coveralls.io/builds/31366647)
   
   Coverage increased (+0.06%) to 89.783% when pulling **293c04285156fdbe48025dd912fb8b56c5f7b7a6 on LinuxClient:feature/add-caching-for-required-charsets** into **cd7787277b170d5f439df03850ca0c574566e539 on apache:master**.
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] coveralls edited a comment on pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #120:
URL: https://github.com/apache/commons-io/pull/120#issuecomment-642062946


   
   [![Coverage Status](https://coveralls.io/builds/31382258/badge)](https://coveralls.io/builds/31382258)
   
   Coverage increased (+0.07%) to 89.785% when pulling **4498f2c5f34d5d230f0ad27f207d69ab0fbfee1a on LinuxClient:feature/add-caching-for-required-charsets** into **cd7787277b170d5f439df03850ca0c574566e539 on apache:master**.
   


----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438293467



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -72,15 +83,7 @@
      * @since 2.5
      */
     public static SortedMap<String, Charset> requiredCharsets() {

Review comment:
       I have now changed it. Could you look over it again? 




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438149732



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -72,15 +83,7 @@
      * @since 2.5
      */
     public static SortedMap<String, Charset> requiredCharsets() {

Review comment:
       Because `Collections.unmodifiableSortedMap(REQUIRED_CHARSETS);` returns a SortedMap. We could use `Collections.unmodifiableMap(REQUIRED_CHARSETS);` or `new TreeMap(REQUIRED_CHARSETS);` to change the return type.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438365111



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -71,16 +83,8 @@
      * @see Charset#availableCharsets()
      * @since 2.5
      */
-    public static SortedMap<String, Charset> requiredCharsets() {
-        // maybe cache?
-        final TreeMap<String, Charset> m = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
-        m.put(StandardCharsets.ISO_8859_1.name(), StandardCharsets.ISO_8859_1);
-        m.put(StandardCharsets.US_ASCII.name(), StandardCharsets.US_ASCII);
-        m.put(StandardCharsets.UTF_16.name(), StandardCharsets.UTF_16);
-        m.put(StandardCharsets.UTF_16BE.name(), StandardCharsets.UTF_16BE);
-        m.put(StandardCharsets.UTF_16LE.name(), StandardCharsets.UTF_16LE);
-        m.put(StandardCharsets.UTF_8.name(), StandardCharsets.UTF_8);
-        return Collections.unmodifiableSortedMap(m);
+    public static Map<String, Charset> requiredCharsets() {
+        return Collections.unmodifiableSortedMap(REQUIRED_CHARSETS);

Review comment:
       Oh, I forgot. I've changed it now.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] LinuxClient commented on a change in pull request #120: Add caching for required charsets

Posted by GitBox <gi...@apache.org>.
LinuxClient commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r438546177



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -71,16 +85,8 @@
      * @see Charset#availableCharsets()
      * @since 2.5
      */
-    public static SortedMap<String, Charset> requiredCharsets() {
-        // maybe cache?
-        final TreeMap<String, Charset> m = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
-        m.put(StandardCharsets.ISO_8859_1.name(), StandardCharsets.ISO_8859_1);
-        m.put(StandardCharsets.US_ASCII.name(), StandardCharsets.US_ASCII);
-        m.put(StandardCharsets.UTF_16.name(), StandardCharsets.UTF_16);
-        m.put(StandardCharsets.UTF_16BE.name(), StandardCharsets.UTF_16BE);
-        m.put(StandardCharsets.UTF_16LE.name(), StandardCharsets.UTF_16LE);
-        m.put(StandardCharsets.UTF_8.name(), StandardCharsets.UTF_8);
-        return Collections.unmodifiableSortedMap(m);
+    public static Map<String, Charset> requiredCharsets() {

Review comment:
       Yeah, I saw it on the Travis CI Log. But it was mentioned by @michael-o above.




----------------------------------------------------------------
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:
users@infra.apache.org

[GitHub] [commons-io] michael-o commented on a change in pull request #120: Add caching for required charsets and cleanup code

Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #120:
URL: https://github.com/apache/commons-io/pull/120#discussion_r437937062



##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -59,6 +59,17 @@
     // correctly and without delay on all Java platforms.
     //
 
+    private static final TreeMap<String, Charset> REQUIRED_CHARSETS = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);

Review comment:
       Use the interface name

##########
File path: src/main/java/org/apache/commons/io/Charsets.java
##########
@@ -72,15 +83,7 @@
      * @since 2.5
      */
     public static SortedMap<String, Charset> requiredCharsets() {

Review comment:
       Why sorted map? TreeMap is sorted already?!




----------------------------------------------------------------
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:
users@infra.apache.org