[2.7] bpo-30409: locale.getpreferredencoding doesn't return result

[seanmccully]

Default behavior of locale.getpreferredencoding doesn't return result.

[AraHaan]

you might need to rename this PR to bpo-30409: locale.getpreferredencoding doesn't return result to fix the issue number part on this PR.

[vstinner]

Please remove "return result" from the other if block (2 lives above).

[AraHaan]

you mean remove line 630?

[vstinner]

Yes.

[seanmccully]

Why? This function would still have inconsistent behavior, if CODESET is set and if on windows it is still returning a value. Are suggesting that this function should not return anything?

[vstinner]

I propose to replace "if ...: ... return result else: ... return result" with "if ...: ... else ...; return result", just factorize the code which is in common. That's all.

[seanmccully]

Not sure entirely if there's any reason for calling setlocale after nl_langinfo?

[AraHaan]

This was going to be my suggested change before the force push:

diff --git a/Lib/locale.py b/Lib/locale.py
index be34c5ddcd..e3674aba48 100644
--- a/Lib/locale.py
+++ b/Lib/locale.py
@@ -610,29 +610,24 @@ else:
         def getpreferredencoding(do_setlocale = True):
             """Return the charset that the user is likely using,
             according to the system configuration."""
+            result = nl_langinfo(CODESET)
+            if not result and sys.platform == 'darwin':
+                # nl_langinfo can return an empty string
+                # when the setting has an invalid value.
+                # Default to UTF-8 in that case because
+                # UTF-8 is the default charset on OSX and
+                # returning nothing will crash the
+                # interpreter.
+                result = 'UTF-8'
             if do_setlocale:
                 oldloc = setlocale(LC_CTYPE)
                 try:
                     setlocale(LC_CTYPE, "")
                 except Error:
                     pass
-                result = nl_langinfo(CODESET)
-                if not result and sys.platform == 'darwin':
-                    # nl_langinfo can return an empty string
-                    # when the setting has an invalid value.
-                    # Default to UTF-8 in that case because
-                    # UTF-8 is the default charset on OSX and
-                    # returning nothing will crash the
-                    # interpreter.
-                    result = 'UTF-8'
 
                 setlocale(LC_CTYPE, oldloc)
-                return result
-            else:
-                result = nl_langinfo(CODESET)
-                if not result and sys.platform == 'darwin':
-                    # See above for explanation
-                    result = 'UTF-8'
+            return result
 
 
 ### Database

I uploaded the diff to bpo too.

[vstinner]

This line is useless. Please remove it.

[seanmccully]

done

[vstinner]

LGTM...

... Oh, I'm really sorry but ask you one more thing, but I missed that it is your first contribution. Pleeeease, can you add yourself to Misc/ACKS? See the header for name order.

[vstinner]

By the way, thank your for going even further than just adding the missing "return result" and for having merged the duplicate code! :-)

[seanmccully]

No problem