From 3c2576a6296c1f80dc97851db93d75554490a9c9 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 1 Sep 2012 11:46:58 +0200 Subject: [PATCH 1/5] Guess what setting you meant based on levenshtein. --- mopidy/utils/settings.py | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/mopidy/utils/settings.py b/mopidy/utils/settings.py index ff449a61..ce245f3b 100644 --- a/mopidy/utils/settings.py +++ b/mopidy/utils/settings.py @@ -12,6 +12,7 @@ from mopidy.utils.log import indent logger = logging.getLogger('mopidy.utils.settings') + class SettingsProxy(object): def __init__(self, default_settings_module): self.default = self._get_settings_dict_from_module( @@ -151,11 +152,17 @@ def validate_settings(defaults, settings): u'Available bitrates are 96, 160, and 320.') if setting not in defaults: - errors[setting] = u'Unknown setting. Is it misspelled?' + errors[setting] = u'Unknown setting.' + suggestion = did_you_mean(setting, defaults) + + if suggestion: + errors[setting] += u' Did you mean %s?' % suggestion + continue return errors + def list_settings_optparse_callback(*args): """ Prints a list of all settings. @@ -167,6 +174,7 @@ def list_settings_optparse_callback(*args): print format_settings_list(settings) sys.exit(0) + def format_settings_list(settings): errors = settings.get_errors() lines = [] @@ -181,8 +189,37 @@ def format_settings_list(settings): lines.append(u' Error: %s' % errors[key]) return '\n'.join(lines) + def mask_value_if_secret(key, value): if key.endswith('PASSWORD') and value: return u'********' else: return value + + +def did_you_mean(setting, defaults): + """Suggest most likely setting based on levenshtein.""" + candidates = [(levenshtein(setting, d), d) for d in defaults] + candidates.sort() + + if candidates[0][0] <= 3: + return candidates[0][1] + return None + + +def levenshtein(a, b, max=3): + "Calculates the Levenshtein distance between a and b." + n, m = len(a), len(b) + if n > m: + return levenshtein(b, a) + + current = xrange(n+1) + for i in xrange(1,m+1): + previous, current = current, [i]+[0]*n + for j in xrange(1,n+1): + add, delete = previous[j]+1, current[j-1]+1 + change = previous[j-1] + if a[j-1] != b[i-1]: + change = change + 1 + current[j] = min(add, delete, change) + return current[n] From 03a7f03bb8bb808646d6e59b46adbe55eb92fb0c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 1 Sep 2012 12:11:09 +0200 Subject: [PATCH 2/5] Style fixes to levenshtein and did_you_mean. --- mopidy/utils/settings.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/mopidy/utils/settings.py b/mopidy/utils/settings.py index ce245f3b..4de7d1cf 100644 --- a/mopidy/utils/settings.py +++ b/mopidy/utils/settings.py @@ -208,18 +208,18 @@ def did_you_mean(setting, defaults): def levenshtein(a, b, max=3): - "Calculates the Levenshtein distance between a and b." + """Calculates the Levenshtein distance between a and b.""" n, m = len(a), len(b) if n > m: return levenshtein(b, a) current = xrange(n+1) - for i in xrange(1,m+1): - previous, current = current, [i]+[0]*n - for j in xrange(1,n+1): - add, delete = previous[j]+1, current[j-1]+1 + for i in xrange(1, m+1): + previous, current = current, [i] + [0] * n + for j in xrange(1, n+1): + add, delete = previous[j] + 1, current[j-1] + 1 change = previous[j-1] if a[j-1] != b[i-1]: - change = change + 1 + change += 1 current[j] = min(add, delete, change) return current[n] From 4e4a209ec319d20f87d4b7cce6f1a6050b5c8a9e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 1 Sep 2012 12:15:08 +0200 Subject: [PATCH 3/5] Fix existing settings tests that did_you_mean broke. --- mopidy/utils/settings.py | 3 +++ tests/utils/settings_test.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mopidy/utils/settings.py b/mopidy/utils/settings.py index 4de7d1cf..34126907 100644 --- a/mopidy/utils/settings.py +++ b/mopidy/utils/settings.py @@ -199,6 +199,9 @@ def mask_value_if_secret(key, value): def did_you_mean(setting, defaults): """Suggest most likely setting based on levenshtein.""" + if not defaults: + return None + candidates = [(levenshtein(setting, d), d) for d in defaults] candidates.sort() diff --git a/tests/utils/settings_test.py b/tests/utils/settings_test.py index 55e1156b..c129b9b5 100644 --- a/tests/utils/settings_test.py +++ b/tests/utils/settings_test.py @@ -23,7 +23,7 @@ class ValidateSettingsTest(unittest.TestCase): result = validate_settings(self.defaults, {'MPD_SERVER_HOSTNMAE': '127.0.0.1'}) self.assertEqual(result['MPD_SERVER_HOSTNMAE'], - u'Unknown setting. Is it misspelled?') + u'Unknown setting. Did you mean MPD_SERVER_HOSTNAME?') def test_not_renamed_setting_returns_error(self): result = validate_settings(self.defaults, From 1f8289a25616a2d15e21d54119297aba2ebb66d2 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 1 Sep 2012 12:22:41 +0200 Subject: [PATCH 4/5] Switch to only importing modules in settings_test. --- tests/utils/settings_test.py | 47 ++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/tests/utils/settings_test.py b/tests/utils/settings_test.py index c129b9b5..1b0c4a2a 100644 --- a/tests/utils/settings_test.py +++ b/tests/utils/settings_test.py @@ -1,8 +1,7 @@ import os -from mopidy import settings as default_settings_module, SettingsError -from mopidy.utils.settings import (format_settings_list, mask_value_if_secret, - SettingsProxy, validate_settings) +import mopidy +from mopidy.utils import settings as setting_utils from tests import unittest @@ -16,29 +15,29 @@ class ValidateSettingsTest(unittest.TestCase): } def test_no_errors_yields_empty_dict(self): - result = validate_settings(self.defaults, {}) + result = setting_utils.validate_settings(self.defaults, {}) self.assertEqual(result, {}) def test_unknown_setting_returns_error(self): - result = validate_settings(self.defaults, + result = setting_utils.validate_settings(self.defaults, {'MPD_SERVER_HOSTNMAE': '127.0.0.1'}) self.assertEqual(result['MPD_SERVER_HOSTNMAE'], u'Unknown setting. Did you mean MPD_SERVER_HOSTNAME?') def test_not_renamed_setting_returns_error(self): - result = validate_settings(self.defaults, + result = setting_utils.validate_settings(self.defaults, {'SERVER_HOSTNAME': '127.0.0.1'}) self.assertEqual(result['SERVER_HOSTNAME'], u'Deprecated setting. Use MPD_SERVER_HOSTNAME.') def test_unneeded_settings_returns_error(self): - result = validate_settings(self.defaults, + result = setting_utils.validate_settings(self.defaults, {'SPOTIFY_LIB_APPKEY': '/tmp/foo'}) self.assertEqual(result['SPOTIFY_LIB_APPKEY'], u'Deprecated setting. It may be removed.') def test_deprecated_setting_value_returns_error(self): - result = validate_settings(self.defaults, + result = setting_utils.validate_settings(self.defaults, {'BACKENDS': ('mopidy.backends.despotify.DespotifyBackend',)}) self.assertEqual(result['BACKENDS'], u'Deprecated setting value. ' + @@ -46,33 +45,33 @@ class ValidateSettingsTest(unittest.TestCase): 'available.') def test_unavailable_bitrate_setting_returns_error(self): - result = validate_settings(self.defaults, + result = setting_utils.validate_settings(self.defaults, {'SPOTIFY_BITRATE': 50}) self.assertEqual(result['SPOTIFY_BITRATE'], u'Unavailable Spotify bitrate. ' + u'Available bitrates are 96, 160, and 320.') def test_two_errors_are_both_reported(self): - result = validate_settings(self.defaults, + result = setting_utils.validate_settings(self.defaults, {'FOO': '', 'BAR': ''}) self.assertEqual(len(result), 2) def test_masks_value_if_secret(self): - secret = mask_value_if_secret('SPOTIFY_PASSWORD', 'bar') + secret = setting_utils.mask_value_if_secret('SPOTIFY_PASSWORD', 'bar') self.assertEqual(u'********', secret) def test_does_not_mask_value_if_not_secret(self): - not_secret = mask_value_if_secret('SPOTIFY_USERNAME', 'foo') + not_secret = setting_utils.mask_value_if_secret('SPOTIFY_USERNAME', 'foo') self.assertEqual('foo', not_secret) def test_does_not_mask_value_if_none(self): - not_secret = mask_value_if_secret('SPOTIFY_USERNAME', None) + not_secret = setting_utils.mask_value_if_secret('SPOTIFY_USERNAME', None) self.assertEqual(None, not_secret) class SettingsProxyTest(unittest.TestCase): def setUp(self): - self.settings = SettingsProxy(default_settings_module) + self.settings = setting_utils.SettingsProxy(mopidy.settings) self.settings.local.clear() def test_set_and_get_attr(self): @@ -83,7 +82,7 @@ class SettingsProxyTest(unittest.TestCase): try: _ = self.settings.TEST self.fail(u'Should raise exception') - except SettingsError as e: + except mopidy.SettingsError as e: self.assertEqual(u'Setting "TEST" is not set.', e.message) def test_getattr_raises_error_on_empty_setting(self): @@ -91,7 +90,7 @@ class SettingsProxyTest(unittest.TestCase): try: _ = self.settings.TEST self.fail(u'Should raise exception') - except SettingsError as e: + except mopidy.SettingsError as e: self.assertEqual(u'Setting "TEST" is empty.', e.message) def test_getattr_does_not_raise_error_if_setting_is_false(self): @@ -177,44 +176,44 @@ class SettingsProxyTest(unittest.TestCase): class FormatSettingListTest(unittest.TestCase): def setUp(self): - self.settings = SettingsProxy(default_settings_module) + self.settings = setting_utils.SettingsProxy(mopidy.settings) def test_contains_the_setting_name(self): self.settings.TEST = u'test' - result = format_settings_list(self.settings) + result = setting_utils.format_settings_list(self.settings) self.assert_('TEST:' in result, result) def test_repr_of_a_string_value(self): self.settings.TEST = u'test' - result = format_settings_list(self.settings) + result = setting_utils.format_settings_list(self.settings) self.assert_("TEST: u'test'" in result, result) def test_repr_of_an_int_value(self): self.settings.TEST = 123 - result = format_settings_list(self.settings) + result = setting_utils.format_settings_list(self.settings) self.assert_("TEST: 123" in result, result) def test_repr_of_a_tuple_value(self): self.settings.TEST = (123, u'abc') - result = format_settings_list(self.settings) + result = setting_utils.format_settings_list(self.settings) self.assert_("TEST: (123, u'abc')" in result, result) def test_passwords_are_masked(self): self.settings.TEST_PASSWORD = u'secret' - result = format_settings_list(self.settings) + result = setting_utils.format_settings_list(self.settings) self.assert_("TEST_PASSWORD: u'secret'" not in result, result) self.assert_("TEST_PASSWORD: u'********'" in result, result) def test_short_values_are_not_pretty_printed(self): self.settings.FRONTEND = (u'mopidy.frontends.mpd.MpdFrontend',) - result = format_settings_list(self.settings) + result = setting_utils.format_settings_list(self.settings) self.assert_("FRONTEND: (u'mopidy.frontends.mpd.MpdFrontend',)" in result, result) def test_long_values_are_pretty_printed(self): self.settings.FRONTEND = (u'mopidy.frontends.mpd.MpdFrontend', u'mopidy.frontends.lastfm.LastfmFrontend') - result = format_settings_list(self.settings) + result = setting_utils.format_settings_list(self.settings) self.assert_("""FRONTEND: (u'mopidy.frontends.mpd.MpdFrontend', u'mopidy.frontends.lastfm.LastfmFrontend')""" in result, result) From e4d425d37aed3ab8a1a7c7b3c1428bb7bf4c03a8 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sat, 1 Sep 2012 12:28:32 +0200 Subject: [PATCH 5/5] Add did you mean tests for settings. - Checks varying degrees of typos until the edit distance becomes to large. - Also updated did you mean to always uppercase it's input so we catch caps errors. --- mopidy/utils/settings.py | 1 + tests/utils/settings_test.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/mopidy/utils/settings.py b/mopidy/utils/settings.py index 34126907..4072f24d 100644 --- a/mopidy/utils/settings.py +++ b/mopidy/utils/settings.py @@ -202,6 +202,7 @@ def did_you_mean(setting, defaults): if not defaults: return None + setting = setting.upper() candidates = [(levenshtein(setting, d), d) for d in defaults] candidates.sort() diff --git a/tests/utils/settings_test.py b/tests/utils/settings_test.py index 1b0c4a2a..7d104969 100644 --- a/tests/utils/settings_test.py +++ b/tests/utils/settings_test.py @@ -217,3 +217,27 @@ class FormatSettingListTest(unittest.TestCase): self.assert_("""FRONTEND: (u'mopidy.frontends.mpd.MpdFrontend', u'mopidy.frontends.lastfm.LastfmFrontend')""" in result, result) + + +class DidYouMeanTest(unittest.TestCase): + def testSuggestoins(self): + defaults = { + 'MPD_SERVER_HOSTNAME': '::', + 'MPD_SERVER_PORT': 6600, + 'SPOTIFY_BITRATE': 160, + } + + suggestion = setting_utils.did_you_mean('spotify_bitrate', defaults) + self.assertEqual(suggestion, 'SPOTIFY_BITRATE') + + suggestion = setting_utils.did_you_mean('SPOTIFY_BITROTE', defaults) + self.assertEqual(suggestion, 'SPOTIFY_BITRATE') + + suggestion = setting_utils.did_you_mean('SPITIFY_BITROT', defaults) + self.assertEqual(suggestion, 'SPOTIFY_BITRATE') + + suggestion = setting_utils.did_you_mean('SPTIFY_BITROT', defaults) + self.assertEqual(suggestion, 'SPOTIFY_BITRATE') + + suggestion = setting_utils.did_you_mean('SPTIFY_BITRO', defaults) + self.assertEqual(suggestion, None)