openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Approve

Seems good to me, 1 minor comment, but I'd be happy for this to be merged

Diff comments:

> 
> === modified file 'openlp/core/common/settings.py'
> --- openlp/core/common/settings.py	2017-11-10 22:50:04 +0000
> +++ openlp/core/common/settings.py	2017-11-16 05:12:46 +0000
> @@ -467,32 +468,38 @@
>          for version in range(current_version, __version__):
>              version += 1
>              upgrade_list = getattr(self, '__setting_upgrade_{version}__'.format(version=version))
> -            for old_key, new_key, rules in upgrade_list:
> +            for old_keys, new_key, rules in upgrade_list:
>                  # Once removed we don't have to do this again. - Can be removed once fully switched to the versioning
>                  # system.
> -                if not self.contains(old_key):
> +                if not isinstance(old_keys, (tuple, list)):
> +                    old_keys = [old_keys]
> +                if any([not self.contains(old_key) for old_key in old_keys]):
> +                    log.warning('One of {} does not exist, skipping upgrade'.format(old_keys))
>                      continue
>                  if new_key:
>                      # Get the value of the old_key.
> -                    old_value = super(Settings, self).value(old_key)
> +                    old_values = [super(Settings, self).value(old_key) for old_key in old_keys]

super().value(old_key) is more concise

>                      # When we want to convert the value, we have to figure out the default value (because we cannot get
>                      # the default value from the central settings dict.
>                      if rules:
> -                        default_value = rules[0][1]
> -                        old_value = self._convert_value(old_value, default_value)
> +                        default_values = rules[0][1]
> +                        if not isinstance(default_values, (list, tuple)):
> +                            default_values = [default_values]
> +                        old_values = [self._convert_value(old_value, default_value)
> +                                      for old_value, default_value in zip(old_values, default_values)]
>                      # Iterate over our rules and check what the old_value should be "converted" to.
> -                    for new, old in rules:
> +                    new_value = None
> +                    for new_rule, old_rule 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 callable(new):
> -                            old_value = new(old_value)
> -                        elif old == old_value:
> -                            old_value = new
> +                        if callable(new_rule):
> +                            new_value = new_rule(*old_values)
> +                        elif old_rule in old_values:
> +                            new_value = new_rule
>                              break
> -                    self.setValue(new_key, old_value)
> -                if new_key != old_key:
> -                    self.remove(old_key)
> -        self.setValue('settings/version', version)
> +                    self.setValue(new_key, new_value)
> +                [self.remove(old_key) for old_key in old_keys if old_key != new_key]
> +            self.setValue('settings/version', version)
>  
>      def value(self, key):
>          """


-- 
https://code.launchpad.net/~raoul-snyman/openlp/settings-upgrade/+merge/333788
Your team OpenLP Core is requested to review the proposed merge of lp:~raoul-snyman/openlp/settings-upgrade into lp:openlp.