openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'openlp/core/common/settings.py'
> --- openlp/core/common/settings.py	2015-01-18 13:39:21 +0000
> +++ openlp/core/common/settings.py	2015-02-13 21:02:19 +0000
> @@ -45,6 +45,13 @@
>          X11_BYPASS_DEFAULT = False
>  
>  
> +def recent_files_conv(value):
> +    if isinstance(value, list):
> +        return value
> +    elif isinstance(value, str):

You probably want to check for a few types. If "basestring" is still around, use that, and also check against QString.

elif isinstance(value, (str, bytes, QtCore.QString):
    ....

> +        return [value]
> +
> +
>  class Settings(QtCore.QSettings):
>      """
>      Class to wrap QSettings.
> @@ -66,10 +73,9 @@
>          The first entry is the *old key*; it will be removed.
>  
>          The second entry is the *new key*; we will add it to the config. If this is just an empty string, we just remove
> -        the old key.
> -
> -        The last entry is a list containing two-pair tuples. If the list is empty, no conversion is made. Otherwise each
> -        pair describes how to convert the old setting's value::
> +        the old key. The last entry is a list containing two-pair tuples. If the list is empty, no conversion is made.
> +        If the first value is callable i.e. a function, the function will be called with the old setting's value.
> +        Otherwise each pair describes how to convert the old setting's value::
>  
>              (SlideLimits.Wrap, True)
>  
> @@ -328,7 +334,7 @@
>          ('general/language', 'core/language', []),
>          ('general/last version test', 'core/last version test', []),
>          ('general/loop delay', 'core/loop delay', []),
> -        ('general/recent files', 'core/recent files', []),
> +        ('general/recent files', 'core/recent files', [(recent_files_conv, None)]),
>          ('general/save prompt', 'core/save prompt', []),
>          ('general/screen blank', 'core/screen blank', []),
>          ('general/show splash', 'core/show splash', []),
> @@ -353,7 +359,7 @@
>  
>          :param default_values: A dict with setting keys and their default values.
>          """
> -        Settings.__default_settings__ = dict(list(default_values.items()) + list(Settings.__default_settings__.items()))
> +        Settings.__default_settings__.update(default_values)
>  
>      @staticmethod
>      def set_filename(ini_file):
> @@ -410,7 +416,9 @@
>                      for new, old in rules:
>                          # If the value matches with the condition (rule), then use the provided value. This is used to
>                          # convert values. E. g. an old value 1 results in True, and 0 in False.
> -                        if old == old_value:
> +                        if callable(new):
> +                            old_value = new(old_value)
> +                        elif old == old_value:
>                              old_value = new
>                              break
>                      self.setValue(new_key, old_value)
> 
> === modified file 'tests/functional/openlp_core_common/test_settings.py'
> --- tests/functional/openlp_core_common/test_settings.py	2015-01-18 13:39:21 +0000
> +++ tests/functional/openlp_core_common/test_settings.py	2015-02-13 21:02:19 +0000
> @@ -25,6 +25,7 @@
>  from unittest import TestCase
>  
>  from openlp.core.common import Settings
> +from tests.functional import patch
>  from tests.helpers.testmixin import TestMixin
>  
>  
> @@ -120,3 +121,19 @@
>  
>          # THEN: An exception with the non-existing key should be thrown
>          self.assertEqual(str(cm.exception), "'core/does not exists'", 'We should get an exception')
> +
> +    def extend_default_settings_test(self):
> +        """
> +        Test that the extend_default_settings method extends the default settings
> +        """
> +        # GIVEN: A patched __default_settings__ dictionary
> +        with patch.dict(Settings.__default_settings__,
> +                   {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 3}, True):
> +
> +            # WHEN: Calling extend_default_settings
> +            Settings.extend_default_settings({'test/setting 3': 4, 'test/extended 1': 1, 'test/extended 2': 2})
> +
> +            # THEN: The _default_settings__ dictionary_ should have the new keys
> +            self.assertEqual(
> +                Settings.__default_settings__, {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 4,
> +                                                'test/extended 1': 1, 'test/extended 2': 2})
> 


-- 
https://code.launchpad.net/~phill-ridout/openlp/bug1416703/+merge/249724
Your team OpenLP Core is subscribed to branch lp:openlp.