Suggestions from code review

yast · Apr 6, 2018 · d60b81c · d60b81c
1 parent b9b7735
commit d60b81c
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 18 deletions.
diff --git a/src/include/nfs/ui.rb b/src/include/nfs/ui.rb
@@ -762,7 +762,7 @@ def edit_legacy?
         "possibility to use old outdated method again.\n\n" \
         "Proceed and edit?"
       )
-      Yast2::Popup.show(msg, buttons: :yes_no) == :yes
+      Yast2::Popup.show(msg, buttons: :continue_cancel) == :continue
     end
   end
 end
diff --git a/src/lib/y2nfs_client/nfs_version.rb b/src/lib/y2nfs_client/nfs_version.rb
@@ -27,34 +27,33 @@ module Y2NfsClient
   # The main goal of the class if to map how the version is configured in the
   # /etc/fstab entry with how that is presented to the user.
   #
-  # Each instance can represent the configuration to enforce a concrete version
-  # or a more generic configuration (i.e. "use any available version").
+  # Each instance is an immutable object that can represent the configuration
+  # to enforce a concrete version or a more generic configuration (i.e. "use
+  # any available version").
   class NfsVersion
     include Yast::I18n
     extend Yast::I18n
     textdomain "nfs"
 
-    ALL_ATTRIBUTES = [
-      [nil,   N_("Any"),     :vers_any, N_("Any (Highest Available)")],
-      ["3",   N_("NFSv3"),   :vers_3,   N_("Force NFSv3")],
-      ["4",   N_("NFSv4"),   :vers_4,   N_("Force NFSv4")],
-      ["4.1", N_("NFSv4.1"), :vers_4_1, N_("Force pNFS (v4.1)")]
-    ].freeze
-    private_constant :ALL_ATTRIBUTES
-
     # Constructor
     def initialize(mntops_value, label, widget_id, widget_text)
-      textdomain "nfs"
-
       @mntops_value = mntops_value
       @label = label
       @widget_id = widget_id
       @widget_text = widget_text
     end
 
+    ALL = [
+      new(nil,   N_("Any"),     :vers_any, N_("Any (Highest Available)")),
+      new("3",   N_("NFSv3"),   :vers_3,   N_("Force NFSv3")),
+      new("4",   N_("NFSv4"),   :vers_4,   N_("Force NFSv4")),
+      new("4.1", N_("NFSv4.1"), :vers_4_1, N_("Force pNFS (v4.1)"))
+    ].freeze
+    private_constant :ALL
+
     # Sorted list of all possible settings
     def self.all
-      ALL_ATTRIBUTES.map { |attrs| new(*attrs) }
+      ALL.dup
     end
 
     # An instance corresponding to a given value in the mount options
@@ -79,14 +78,14 @@ def self.for_mntops_value(value)
 
     # Short localized label to represent the instance in listings
     #
-    # @return [String]
+    # @return [String] very likely, a frozen string
     def label
       _(@label)
     end
 
     # Localized text to use in a widget representing the instance
     #
-    # @return [String]
+    # @return [String] very likely, a frozen string
     def widget_text
       _(@widget_text)
     end

diff --git a/src/modules/NfsOptions.rb b/src/modules/NfsOptions.rb
@@ -277,8 +277,8 @@ def legacy?(options)
     # @return [String, nil] contains the whole 'option=value' string
     def relevant_version_option(option_list)
       # According to manual tests and documentation, none of the forms has higher precedence.
-      # Use #reverse because in case of conflicting options, the latest one is used by mount
-      option_list.reverse.find do |opt|
+      # Use #reverse_each because in case of conflicting options, the latest one is used by mount
+      option_list.reverse_each.find do |opt|
         version_option?(opt)
       end
     end