[WIP] refactoring autoconverted ruby code into ruby code #40

vpereira · 2016-09-01T10:16:47Z

Hi, I refactored some autoconverted code. I didn't change any functionality. Just refactoring.

mvidner · 2016-09-01T14:52:57Z

Hi, I refactored some autoconverted code. I didn't change any functionality. Just refactoring.

Sorry, in the past we have seen that such refactoring often leads to bugs. So it would be good to mention this in package/*.changes.

It appears you have user RuboCop for this. Why not commit also the configuration file?

vpereira · 2016-09-01T14:59:12Z

sure, i used a rubocop configuration used by yast. I will attach it.

jreidinger · 2016-10-04T10:07:54Z

rubocop_yast_style.yml

+
+# do not force %r
+Style/RegexpLiteral:
+  Enabled: false


thanks for you ruby style guide, but better to reference it. See how other modules do it: e.g. https://github.com/yast/yast-bootloader/blob/master/.rubocop.yml#L3

jreidinger · 2016-10-04T10:15:06Z

src/clients/security.rb

+      current = if levels.empty?
+                  :custom
+                else
+                  levels.last # mimic the old implementation


this is in fact wrong, as levels contain pair of [key, level] and old one use only key.
so how code can look like:

current = @Levels.select{ |_key, level| level == Security.Settings }.keys.last || :custom

jreidinger · 2016-10-04T10:16:35Z

src/clients/security.rb

      end
-      false
+      Security.modified


this in fact is not same as previous code as it can return true, when something else set Security.modified to true and here it is not set

and why we return always false? there is a reason for that?

I expect that logic is that we return false to indicate that this action do not change anything.

jreidinger · 2016-10-04T10:18:39Z

src/clients/security.rb

-          Security.Settings,
-          "PASSWD_REMEMBER_HISTORY",
-          Ops.get_string(options, "remember", "0")
-        )


this looks like removed without replacemend.

i think i covered the case in my change, no?

As I see, maybe the problem is that I'm converting the options["remember"] to int?

yes, this code uses string, not sure how hard it is

I changed it already to use strings. I think there is no problem, but consistence is good.

jreidinger · 2016-10-04T10:19:50Z

src/clients/security_auto.rb

-          @param = Convert.to_map(WFM.Args(1))
+      if !Yast::WFM.Args.empty?
+        @func = Yast::WFM.Args[0]
+        if Yast::WFM.Args.length > 1 && Yast::WFM.Args[1].is_a?(Hash)


in fact second option is enough, because if length is 1 or less, then Yast::WFM.Args[1] returns nil and nil.is_a?(Hash) returns false

jreidinger · 2016-10-04T10:20:33Z

src/clients/security_auto.rb

@@ -90,37 +90,37 @@ def main
        end
        @ret = Security.Import(
          Map.KeysToUpper(
-            Convert.convert(@param, :from => "map", :to => "map <string, any>")
+            Convert.convert(@param, from: "map", to: "map <string, any>")


in fact whole this convert can be dropeed as implementation do not distinct between map and specialized map.

so simple @ret = Security.Import(Map.KeysToUpper(@param)) can be used

jreidinger · 2016-10-04T10:21:12Z

src/clients/security_auto.rb

-            :from => "map",
-            :to   => "map <string, any>"
+            from: "map",
+            to:   "map <string, any>"


jreidinger · 2016-10-04T10:21:53Z

src/clients/security_auto.rb

@@ -131,8 +131,8 @@ def main
      Builtins.y2milestone("Security auto finished")
      Builtins.y2milestone("----------------------------------------")

-      deep_copy(@ret) 
-
+      # is it needed?


no, it is not needed as @ret is always assigned and from fresh copy.

jreidinger · 2016-10-04T10:22:53Z

src/include/security/dialogs.rb

@@ -354,7 +352,7 @@ def DisplayHelpPopup(help_id)
      if help_id == "MANDATORY_SERVICES"
        missing = Security.MissingMandatoryServices

-        if missing != nil && missing != []
+        if !missing.nil? && missing != []


in fact simple if missing && !missing.empty? can be used.

jreidinger · 2016-10-04T10:23:23Z

src/include/security/dialogs.rb

@@ -374,7 +371,7 @@ def DisplayHelpPopup(help_id)
      elsif help_id == "EXTRA_SERVICES"
        extra = Security.ExtraServices

-        if extra != nil && extra != []
+        if !extra.nil? && extra != []


jreidinger · 2016-10-04T10:23:32Z

src/include/security/dialogs.rb

@@ -384,7 +381,7 @@ def DisplayHelpPopup(help_id)
        end
      end

-      if help != nil && help != ""
+      if !help.nil? && help != ""


jreidinger · 2016-10-04T10:24:26Z

src/include/security/routines.rb

@@ -89,7 +89,7 @@ def settings2widget(_ID)
      # "Widget" == "IntField"
      if widget == "IntField"
        intval = Builtins.tointeger(value)
-        intval = 0 if intval == nil
+        intval = 0 if intval.nil?


intval ||= 0 is simplier

proposed by the rubycop :)

I know, as my proposal won't work if intval will be boolean, but it is not, so it is safer and nicer.

jreidinger · 2016-10-04T10:24:40Z

src/include/security/routines.rb

@@ -112,15 +112,15 @@ def settings2widget(_ID)
        # string|list it
        Builtins.y2debug("li=%1 (%2)", li, i)
        it = Ops.get(li, i)
-        it = "" if it == nil
+        it = "" if it.nil?


it ||= ""

jreidinger · 2016-10-04T10:26:55Z

src/include/security/routines.rb

-        else
-          new = "no"
-        end
+        new = if ret == true


new = ret ? "yes" : "no"

jreidinger · 2016-10-04T10:30:23Z

src/modules/Security.rb

@@ -268,9 +262,9 @@ def PollAbort
    end

    # Abort function
-    # @return blah blah lahjk
+    # @return false


this is in fact not true, as first statement is return. it should be return true if operation should be aborted

jreidinger · 2016-10-04T10:30:53Z

src/modules/Security.rb

    def Abort
-      return Builtins.eval(@AbortFunction) == true if @AbortFunction != nil
+      return Builtins.eval(@AbortFunction) == true if !@AbortFunction.nil?


return Builtins.eval(@AbortFunction) == true if @AbortFunction

jreidinger · 2016-10-04T10:31:40Z

src/modules/Security.rb

+                    Integer(val)
+                  rescue
+                    nil
+                  end


why is this better then before?

proposed by the rubycop :)

jreidinger · 2016-10-04T10:31:53Z

src/modules/Security.rb

+                Integer(@Settings.fetch("kernel.sysrq", "0"))
+              rescue
+                nil
+              end


proposed by the rubycop :)

jreidinger · 2016-10-04T10:33:10Z

src/modules/Security.rb

@@ -731,35 +733,30 @@ def Import(settings)
    # (For use by autoinstallation.)
    # @return [Hash] Dumped settings (later acceptable by Import ())
    def Export
-      Builtins.eval(@Settings)
+      @Settings if @Settings.is_a?(Hash)


well, here is bug, that eval do copy. Also check for hash is not needed, so it should look like

deep_copy(@Settings)

jreidinger · 2016-10-04T10:33:54Z

src/modules/Security.rb

-      Builtins.foreach(@do_not_test) do |key|
-        settings = Builtins.remove(settings, key)
-      end
+      @do_not_test.each { |k| settings.delete k }


what about settings = settings - @do_not_test ?

or even better settings -= @do_not_test

hm.. settings is a hash, no? Plain hashes don't have the "-" method implemented, I'm not sure if I can do it.

ok, if it is Hash, then delete is right approach

jreidinger · 2016-10-04T10:36:40Z

src/modules/Security.rb

-          _("Current Security Level: %1"),
-          Ops.get(@LevelsNames, Convert.to_string(current), "")
-        )
+        summary = _("Current Security Level: #{@LevelsNames[current]}")


no, this won't work, as _ method get extrapolated string, which is wrong. It have to look in ruby like
summary = _("Current Security Level: %s") % @LevelsNames[current]

jreidinger · 2016-10-05T06:41:15Z

src/clients/security_auto.rb

-
-      # is it needed?
-      deep_copy(@ret)
-      # EOF


this is in fact wrong as you kill return value. I just said that deep_copy is not needed, so still you need to have here @ret otherwise it return nil, causing bugs.

jreidinger · 2016-10-05T06:41:45Z

src/include/security/routines.rb

@@ -112,7 +112,7 @@ def settings2widget(_ID)
        # string|list it
        Builtins.y2debug("li=%1 (%2)", li, i)
        it = Ops.get(li, i)
-        it = "" if it.nil?
+        it = ||= ""


this is wrong, it should be it ||=

vpereira · 2016-10-05T12:43:42Z

I fixed almost all issues. Rubocop has just three complains related with useless assignment. Could you please evaluate them?

src/clients/security.rb:180:7: W: Lint/UselessAssignment: Useless assignment to variable - current. 
      current = :custom
      ^^^^^^^
src/include/security/complex.rb:138:9: W: Lint/UselessAssignment: Useless assignment to variable - cur. 
        cur = UI.QueryWidget(Id(:rb), :CurrentButton)
        ^^^
src/include/security/dialogs.rb:289:9: W: Lint/UselessAssignment: Useless assignment to variable - current_value. 
        current_value = Ops.get(
        ^^^^^^^^^^^^^

jreidinger · 2016-10-05T13:14:01Z

@vpereira OK, I evaluated them and result is:

src/clients/security.rb:180 - valid as you set custom few lines lower as default, so please remove it
src/include/security/complex.rb:138 - also valid as when cur is used, it is few lines below, so please remove it
src/include/security/dialogs.rb:289 - also valid, looks like old code, now it uses label_mapping for label instead of name, so also can be removed

jreidinger · 2017-02-28T11:09:23Z

src/clients/security.rb

        Security.modified = true
-        return true


is it valid? what is reason for this change?

jreidinger · 2017-02-28T11:34:52Z

src/lib/security/display_manager.rb

@@ -19,14 +19,15 @@
 # current contact information at www.suse.com.
 # ------------------------------------------------------------------------------
 #
-
+# rubocop:disable Style/MutableConstant


why it is needed? why not simply freeze array?

ah, below we in fact modify it. It does not look correct for me.

jreidinger · 2017-02-28T11:37:31Z

src/modules/Security.rb

+                                YAML.load_file(srv_file)
+                              rescue
+                                {}
+                              end) : {}


to be honest this looks horrible. why not use

srv_lists = {} if srv_file begin srv_lists = YAML.load_file(srv_file) rescue => e log.warn "Failed to load yaml file: #{e.message}" end end

proposed by rubycop ;)

I know, but as I said it is really ugly and I think with my code rubocop will be also happy ;)

thats matter of taste. I can use your approach. However I prefer the "functional" style returning and assigning the value of if.

i'm doing as you suggested @jreidinger

jreidinger · 2017-02-28T11:38:51Z

ok, I added few mote notes, that looks wrong for me. Otherwise it looks good.

lslezak · 2017-04-03T13:10:30Z

Ping, @vpereira could you look at the @jreidinger 's comments?

vpereira · 2017-04-03T17:24:44Z

hi, I will check it ASAP, sorry for the delay

…

On Mon, Apr 3, 2017 at 3:10 PM, Ladislav Slezák ***@***.***> wrote: Ping, @vpereira <https://github.com/vpereira> could you look at the @jreidinger <https://github.com/jreidinger> 's comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#40 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACSKrbylE6l0hH6ZlW3E4KaPcPH2DK4ks5rsO_HgaJpZM4JyiEN> .

jreidinger

only missing answer is one for https://github.com/yast/yast-security/pull/40/files#r103430182 to be sure change of return value is valid.

teclator · 2018-04-09T10:53:06Z

@vpereira @jreidinger it seems this PR is just waiting for the missing answer about a change or returned values.

@vpereira do you plan to work on it ? if not, as it is only a cleanup and no bug related I would close it until we are really able to finish it

vpereira · 2018-04-09T13:52:12Z

hi @teclator , the communication in this PR didn't work so well.. some long period of silence between the commits makes hard for someone sporadically contributing to the codestream to come back and work on that. There are still a question from my side, without answer. Beside it, maybe some commits should be cherry-picked, since it was started in 2016 and probably we updated already our rubocop rules.. Please advice what should be done here.

teclator · 2018-04-11T11:27:03Z

@vpereira up to you. I mean, you can rebase with master fixing the conflicts and check rubocop. If you do not have time or do not plan to work on it then taking in account that we are in RC phase and that it is just a cleanup and do not fix any bug at all I would close it.

teclator · 2018-04-13T12:22:13Z

Closing it by now

refactoring autoconverted ruby code into ruby code

37c8b03

vpereira added 2 commits September 1, 2016 16:59

added rubocop_yast_style.yml

8aa8cf5

added changes to package/yast2-security.changes

a5ce056

jreidinger reviewed Oct 4, 2016

View reviewed changes

src/clients/security_auto.rb Outdated

:from => "map",

:to => "map <string, any>"

from: "map",

to: "map <string, any>"

Copy link

Member

jreidinger Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

jreidinger reviewed Oct 4, 2016

View reviewed changes

src/include/security/routines.rb Outdated

else

new = "no"

end

new = if ret == true

Copy link

Member

jreidinger Oct 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new = ret ? "yes" : "no"

jreidinger reviewed Oct 4, 2016

View reviewed changes

jreidinger reviewed Oct 5, 2016

View reviewed changes

vpereira force-pushed the master branch from 1b1f4ae to 126b874 Compare October 5, 2016 07:13

fixed some changes proposed by jreidinger

e7a1a07

vpereira force-pushed the master branch from c4fcecc to e7a1a07 Compare October 5, 2016 08:37

fixed rubocop issues

7aed7c3

fixed all rubocops offenses

d9e8a4f

jreidinger reviewed Feb 28, 2017

View reviewed changes

src/clients/security.rb

Security.modified = true

return true

Copy link

Member

jreidinger Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it valid? what is reason for this change?

jreidinger reviewed Feb 28, 2017

View reviewed changes

teclator changed the title ~~refactoring autoconverted ruby code into ruby code~~ [WIP] refactoring autoconverted ruby code into ruby code Apr 6, 2017

fixed suggestion from jreidinger

8b37930

jreidinger requested changes Apr 11, 2017

View reviewed changes

teclator closed this Apr 11, 2018

teclator reopened this Apr 11, 2018

teclator closed this Apr 13, 2018

[WIP] refactoring autoconverted ruby code into ruby code #40

[WIP] refactoring autoconverted ruby code into ruby code #40

Conversation

vpereira commented Sep 1, 2016

mvidner commented Sep 1, 2016

vpereira commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpereira commented Oct 5, 2016

jreidinger commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreidinger commented Feb 28, 2017

lslezak commented Apr 3, 2017

vpereira commented Apr 3, 2017 via email

jreidinger left a comment

Choose a reason for hiding this comment

teclator commented Apr 9, 2018

vpereira commented Apr 9, 2018

teclator commented Apr 11, 2018

teclator commented Apr 13, 2018