Code review fixes

library/control/src/modules/ProductFeatures.rb

@@ -213,10 +213,12 @@ def GetFeature(section, feature)
     # @return [String] the feature value
     def GetStringFeature(section, feature)
       value = GetFeature(section, feature)
-      if Ops.is_string?(value)
+
+      case value
+      when ::String
         value
-      elsif Ops.is_boolean?(value)
-        Convert.to_boolean(value) ? "yes" : "no"
+      when true, false
+        value ? "yes" : "no"
       else
         Builtins.sformat("%1", value)
       end

library/control/src/modules/WorkflowManager.rb

@@ -119,16 +119,19 @@ def main
     # @param [String] which_steps (type) of finish ("before_chroot", "after_chroot" or "before_umount")
     # @return [Array<String>] steps to be called ...see which_steps parameter
     def GetAdditionalFinishSteps(which_steps)
-      if which_steps == "before_chroot"
-        deep_copy(@additional_finish_steps_before_chroot)
-      elsif which_steps == "after_chroot"
-        deep_copy(@additional_finish_steps_after_chroot)
-      elsif which_steps == "before_umount"
-        deep_copy(@additional_finish_steps_before_umount)
+      ret = case which_steps
+      when "before_chroot"
+        @additional_finish_steps_before_chroot
+      when "after_chroot"
+        @additional_finish_steps_after_chroot
+      when "before_umount"
+        @additional_finish_steps_before_umount
       else
         Builtins.y2error("Unknown FinishSteps type: %1", which_steps)
         nil
       end
+
+      deep_copy(ret)
     end
 
     # Stores the current ProductControl settings as the initial settings.
@@ -575,11 +578,11 @@ def ReplaceProposalModule(proposal, old, new)
 
           next deep_copy(new) unless Ops.is_map?(m)
 
-          next Builtins.maplist(new) do |it|
+          Builtins.maplist(new) do |it|
             Builtins.union(Convert.to_map(m), "name" => it)
           end
         else
-          next [m]
+          [m]
         end
       end
 
@@ -597,7 +600,7 @@ def ReplaceProposalModule(proposal, old, new)
             modules2 = Builtins.maplist(
               Ops.get_list(tab, "proposal_modules", [])
             ) do |m|
-              next (m == old) ? deep_copy(new) : [m]
+              (m == old) ? deep_copy(new) : [m]
             end
 
             Ops.set(tab, "proposal_modules", Builtins.flatten(modules2))
@@ -756,7 +759,7 @@ def ReplaceWorkflowModule(workflow, old, new, domain, keep)
 
         new_list = Builtins.add(new_list, m) if keep
 
-        next deep_copy(new_list)
+        deep_copy(new_list)
       end
 
       if !found

library/general/src/modules/Hooks.rb

@@ -133,8 +133,8 @@ def to_s
       end
 
       def verify!
-        return path if path.exist?
-        raise "Hook search path #{path} does not exists"
+        raise "Hook search path #{path} does not exists" unless path.exist?
+        path
       end
 
     private

library/general/src/modules/ValueBrowser.rb

@@ -93,7 +93,7 @@ def BrowseTreeHelper(variable, indent)
       variable = deep_copy(variable)
       simple = FormatSimpleType(variable, indent)
 
-      return Item(simple) if !simple.nil?
+      return Item(simple) unless simple.nil?
 
       if Ops.is_list?(variable)
         items = []

library/network/src/modules/CWMFirewallInterfaces.rb

@@ -252,7 +252,7 @@ def Selected2Opened(ifaces, _nm_ifaces_have_to_be_selected)
         next [] if ifaces_left_explicitely == []
 
         # Hmm, some interfaces left
-        next deep_copy(ifaces_also_supported_by_any)
+        deep_copy(ifaces_also_supported_by_any)
       end
 
       Builtins.y2milestone("Ifaces touched: %1", iface_groups)
@@ -633,8 +633,6 @@ def CheckPossbilityToChangeFirewall(new_status)
         @buggy_ifaces = deep_copy(@all_interfaces)
         return false
       end
-
-      false
     end
 
     # Init function of the widget

library/network/src/modules/NetworkService.rb

@@ -274,7 +274,7 @@ def StartStop
     #
     # @return [Boolean] continue
     def ConfirmNetworkManager
-      return true unless !@already_asked_for_NetworkManager && network_manager?
+      return true if @already_asked_for_NetworkManager || !network_manager?
 
       # TRANSLATORS: pop-up question when reading the service configuration
       cont = Popup.ContinueCancel(

library/packages/src/modules/PackageSystem.rb

@@ -410,7 +410,7 @@ def CheckAndInstallPackages(packages)
     def CheckAndInstallPackagesInteractive(packages)
       packages = deep_copy(packages)
       success = CheckAndInstallPackages(packages)
-      return success if success
+      return true if success
 
       if !LastOperationCanceled()
         if Mode.commandline

library/sequencer/src/modules/Sequencer.rb

@@ -133,29 +133,29 @@ def WS_check(aliases, sequence)
       # check all aliases in sequence
       ret0 = Builtins.maplist(sequence) do |key, val|
         if key == "ws_start"
-          next true unless !Ops.is_symbol?(val) && Ops.get(aliases, val).nil?
+          next true if Ops.is_symbol?(val) || !Ops.get(aliases, val).nil?
 
           Builtins.y2error(2, "sequencer check: alias not found: %1", val)
-          next false
+          false
         elsif Ops.get(aliases, key).nil?
           Builtins.y2error(2, "sequencer check: alias not found: %1", key)
-          next false
+          false
         elsif !Ops.is_map?(val)
           Builtins.y2error(2, "sequencer check: not a map: %1 %2", key, val)
-          next false
+          false
         else
           ret1 = Builtins.maplist(Convert.to_map(val)) do |k, v|
             if !Ops.is_symbol?(k)
               Builtins.y2error(2, "sequencer check: not a symbol: %1", k)
-              next false
+              false
             elsif !Ops.is_symbol?(v) && Ops.get(aliases, v).nil?
               Builtins.y2error(2, "sequencer check: alias not found: %1", v)
-              next false
+              false
             else
-              next true
+              true
             end
           end
-          next ret1.all? { |v| v }
+          ret1.all?
         end
       end
       ret = Builtins.flatten([ret, ret0])

library/system/src/modules/FileChanges.rb

@@ -173,22 +173,19 @@ def CheckFiles(files)
 
       return true unless Ops.greater_than(Builtins.size(files), 0)
 
-      msg = if Ops.greater_than(Builtins.size(files), 1)
+      msg = n_(
         # Continue/Cancel question, %1 is a coma separated list of file names
-        _("Files %1 have been changed manually.\nYaST might lose some of the changes")
-      else
+        _("Files %1 have been changed manually.\nYaST might lose some of the changes"),
         # Continue/Cancel question, %1 is a file name
-        _("File %1 has been changed manually.\nYaST might lose some of the changes.\n")
-      end
+        _("File %1 has been changed manually.\nYaST might lose some of the changes.\n"),
+        files.size
+      )
 
       msg = Builtins.sformat(msg, Builtins.mergestring(files, ", "))
       popup_file = "/filechecks_non_verbose"
 
-      return true unless {} ==
-          SCR.Read(
-            path(".target.stat"),
-            Ops.add(Directory.vardir, popup_file)
-          )
+      stat = SCR.Read(path(".target.stat"), Ops.add(Directory.vardir, popup_file))
+      return true unless stat == {}
 
       content = VBox(
         Label(msg),
@@ -227,15 +224,14 @@ def CheckNewCreatedFiles(files)
 
       return true unless !new_files.empty?
 
-      # Continue/Cancel question, %s is a file name
-      msg = _("File %s has been created manually.\nYaST might lose this file.")
-      if new_files.size > 1
+      msg = n_(
+        # Continue/Cancel question, %s is a file name
+        "File %s has been created manually.\nYaST might lose this file.",
         # Continue/Cancel question, %s is a comma separated list of file names
-        msg = _(
-          "Files %s have been created manually.\nYaST might lose these files."
-        )
-      end
-      msg = msg % new_files.join(", ")
+        "Files %s have been created manually.\nYaST might lose these files.",
+        new_files.size
+      ) % new_files.join(", ")
+
       popup_file = "/filechecks_non_verbose"
       popup_file_path = File.join(Directory.vardir, popup_file)
 

library/types/src/modules/IP.rb

@@ -266,7 +266,7 @@ def CheckNetwork4(network)
     #   CheckNetwork("::1/257") -> false
     def CheckNetwork6(network)
       generic_check = CheckNetworkShared(network)
-      return generic_check if !generic_check.nil?
+      return generic_check unless generic_check.nil?
 
       # 2001:db8:0::1/64
       if network =~ Regexp.new("^[" + @ValidChars6 + "]+/[" + Netmask.ValidChars6 + "]*$")

library/types/src/modules/String.rb

@@ -735,7 +735,7 @@ def FormatFilename(file_path, len)
         break unless len # funny backward compatibility that for nil len remove one element
 
         # the size is OK
-        break unless ret.size > len
+        break if ret.size <= len
 
         # still too long, remove the ellipsis and start a new iteration
         dir.delete(ellipsis)

library/types/src/modules/TypeRepository.rb

@@ -52,7 +52,7 @@ def main
     def is_a(value, type)
       value = deep_copy(value)
       validator = Ops.get(@types, type)
-      return validator.call(value) if !validator.nil?
+      return validator.call(value) if validator
 
       Builtins.y2error("Request to validate unknown type %1", type)
       false