New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
1445387: Set locale fact to Unknown if value cannot be determined #1605
Conversation
@@ -67,7 +67,10 @@ def get_all(self): | |||
host_facts.update(firmware_info_dict) | |||
|
|||
locale_info = {} | |||
locale_info['system.default_locale'] = ".".join(locale.getdefaultlocale()) | |||
effective_locale = 'Unknown' | |||
if locale.getdefaultlocale()[0] is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible for locale.getdefaultlocale() to return ("Something_not_none", None)?
Perhaps the following to be safe:
if all(local_entry is not None for locale_entry in locale.getdefaultlocale()):
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrmmm. Good question actually... I tried
$ LC_ALL=en_us python -c 'import locale; print locale.getdefaultlocale()'
('en_US', 'ISO8859-1')
but lemme code for that just in case...
class HostCollectorTest(unittest.TestCase): | ||
|
||
@mock.patch('locale.getdefaultlocale') | ||
def test_unknown_locale(self, mock_locale): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we think it's useful, I would suggest adding one more test for the case of ("Not_None", None) being returned from locale.getdefaultlocale()
That is if the language code from `getdefaultlocale`[1] is `None`, then special case this as 'Unknown'. If encoding is `None`, then it won't be included. [1]: https://docs.python.org/2/library/locale.html#locale.getdefaultlocale
387aef3
to
9c43204
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
That is if the language code from
getdefaultlocale
1 isNone
, thenspecial case this as 'Unknown'.