eyeonus / Trade-Dangerous

Mozilla Public License 2.0
96 stars 31 forks source link

Locale changes break on systems with certain locales, in general unnecessary #149

Closed ultimatespirit closed 2 months ago

ultimatespirit commented 2 months ago

https://github.com/eyeonus/Trade-Dangerous/blob/f71dca3b1d8a98edc5b5adf7964b56473b87e088/tradedangerous/plugins/eddblink_plug.py#L38

Y'all used "en" in lang here, but that just checks that "en" is in the output at all. This will break if the locale happens to contain en in it that isn't english, like the valid extension to the locale format allowing for an @location extension. This causes it to depend on sort order and what locales are installed on the system for it to actually get an english locale.

All locales are required to follow the following format: LanguageCode_CountryCode.encoding[@modifier]

You could just search for en_..\.utf8. Either using the re module or just implementing the string comparison properly.

$ git diff
diff --git a/tradedangerous/plugins/eddblink_plug.py b/tradedangerous/plugins/eddblink_plug.py
index 4ab2e2e..196df46 100644
--- a/tradedangerous/plugins/eddblink_plug.py
+++ b/tradedangerous/plugins/eddblink_plug.py
@@ -13,6 +13,7 @@ import certifi
 import csv
 import datetime
 import os
+import re
 import requests
 import sqlite3
 import ssl
@@ -22,23 +23,20 @@ import platform

 if typing.TYPE_CHECKING:
-    from typing import Optional
+    from typing import Optional, Iterable
     from .. tradeenv import TradeEnv

 # Find the first English UTF-8 locale for use in parsing timestamps.
-LOCALE = None
 if platform.system() == 'Windows':
-    for lang in locale.windows_locale.values():
-        if "en" in lang:
-            LOCALE = lang
-            break
+    pat = re.compile(r"^en_..")
+    locales: Iterable[str] = locale.windows_locale.values()
 else:
     # for other operating systems
-    for lang in locale.locale_alias.values():
-        if "en" in lang and "UTF-8" in lang:
-            LOCALE = lang
-            break
-if not LOCALE:
+    pat = re.compile(r"^en_..\.UTF-8")
+    locales = locale.locale_alias.values()
+LOCALE = next((lang for lang in locales if pat.search(lang) is not None), None)
+
+if LOCALE is None:
     raise PluginException(
         "Unable to find compatible locale.\n" +

...however, the premise that "just use the first english we find" is inherently flawed. On my system the above patch will find en_AG.utf-8 first and then fail in the setlocale() call. In fact, every thing it finds will fail save for the locale I actually installed (the output of python's locale function does not equal the available locales on the system).

Sure, you could solve this with a try: except loop trying each option till one succeeds but... why's this even being done? To... parse an http header it looks like.

The actual solution

Just don't rely on locales then, it's guaranteed to be a buggy mess that will likely break. The http header format for "last modified" is a defined standard, and the email module exists. Or roll your own parser of the format if you want, it's pretty simple.

email solution

from email.utils import parsedate_to_datetime
...
        last_modified = response.headers.get("last-modified")
        dump_mod_time = parsedate_to_datetime(last_modified).timestamp()

Simple as that, no locale shenanigans involved. If the header returned is invalid it'll raise ValueError and be at worst as broken as the current implementation is, but it won't be a per-user problem there but rather the http server's format.