Set correct order for singleton methods of URI with refactor normalized_path method

[Mifrill]

Apply proper order for self methods (https://github.com/rubocop/ruby-style-guide#consistent-classes)

[luke-hill]

you can make this a regular assignment and delete the line above

[Mifrill]

@luke-hill actually - no. We can't change it coz of not obvious end until mod.nil? here: #319 (comment)

I checked it locally, unfortunately, we get an infinite loop without this line.

However, this is not my code, actually, It became from the previous authors (igrigorik), while looks short becomes unreadable. In this PR I just moved it.
Anyway, thank you for your time.

[Mifrill]

sorry, silly me. you are right. fixed it. Thank you

[dentarg]

@Mifrill if you rebase this on latest master you might make the coverage check happy

[Mifrill]

@dentarg can you re-run the Hound checks, please? coz it seems like it lags here.

[dentarg]

I don't have access to Hound, sorry

[Mifrill]

It looks like %r{} syntax fails Profile Memory Allocation check, so probably, good to change Style/RegexpLiteral rule with EnforcedStyle: slashes option, we also using slashes in all other cases

[dentarg]

Did this start out as just moving some method definitions above the protected keyword?

Would be nice to get the changes without moving the code around in a separate PR, to see a more clear diff

[Mifrill]

@dentarg yes, the original intention was about put strict order for protected methods.

as you can see changes for normalized_path method it does not contain logic changes:

       normalized_path = path.dup
+
+      loop do
-      begin
-        mod = nil
         mod ||= normalized_path.gsub!(RULE_2A, SLASH)
         pair = normalized_path.match(RULE_2B_2C)
+
+        if pair
+          parent  = pair[1]
+          current = pair[2]
+        else
+          parent  = nil
+          current = nil
+        end
+
+        regexp = "/#{Regexp.escape(parent.to_s)}/\\.\\./|"
+        regexp += "(/#{Regexp.escape(current.to_s)}/\\.\\.$)"
+
-        parent, current = pair[1], pair[2] if pair
         if pair && ((parent != SELF_REF && parent != PARENT) ||
           (current != SELF_REF && current != PARENT))
+          mod ||= normalized_path.gsub!(Regexp.new(regexp), SLASH)
-          mod ||= normalized_path.gsub!(
-            Regexp.new(
-              "/#{Regexp.escape(parent.to_s)}/\\.\\./|" +
-                "(/#{Regexp.escape(current.to_s)}/\\.\\.$)"
-            ), SLASH
-          )
         end
 
         mod ||= normalized_path.gsub!(RULE_2D, EMPTY_STR)
         # Non-standard, removes prefixed dotted segments from path.
         mod ||= normalized_path.gsub!(RULE_PREFIXED_PARENT, SLASH)
+
+        break if mod.nil?
+      end
-      end until mod.nil?
 
       normalized_path
     end

[Mifrill]

@dentarg lemme elaborate.
The protected keyword doe not affect for singleton method in case of use it this way:

irb(main):001:1* class Hello
irb(main):002:1*   protected
irb(main):003:2*   def self.world
irb(main):004:1*   end
irb(main):005:0> end
=> :world
irb(main):006:0> Hello.world
=> nil

it only makes sense, when we protect the singleton methods, like this way:

irb(main):007:1* class HelloProtected
irb(main):008:2*   class << self
irb(main):009:2*     protected
irb(main):010:3*     def world
irb(main):011:2*     end
irb(main):012:1*   end
irb(main):013:0> end
=> :world
irb(main):014:0> HelloProtected.world
Traceback (most recent call last):
        4: from /usr/bin/irb:23:in `<main>'
        3: from /usr/bin/irb:23:in `load'
        2: from /usr/lib/ruby/gems/2.7.0/gems/irb-1.2.1/exe/irb:11:in `<top (required)>'
        1: from (irb):14
NoMethodError (protected method `world' called for HelloProtected:Class)

so, according to style-guide the self methods should be moved upper on class declaration. That's the intention of this PR.

About refactoring normalized_path method 2 things here:

1. end until mod.nil? was changed due to recommendation:
   https://github.com/rubocop/ruby-style-guide#loop-with-break
2. local variables parent/current was changed due to IDE recommendations: may be uninitialized
   

@dentarg are you still think that: it is worth it to make dedicated PR for a "more clear diff"?

[dentarg]

Yes, I think it makes sense to try to change only one thing per PR and keep diffs minimal :) It makes review easier.

Thanks for the clear explanations. (I understand some of the changes came from the style bot started yelling, that setup is not ideal, but it is what it is, I don't think I can change it)

[Mifrill]

@dentarg okay, I see your point about minimal changes and totally agree with that. I also believe we should have "green" PR to proceed with the merge (that's why we set up PR checks). So, "style bot started yelling" to help us improve codebase, so those @houndci-bot suggestions for this PR look good:
For example:
#319 (comment)
Exactly about this https://github.com/rubocop/ruby-style-guide#loop-with-break

Lint/Loop: Use Kernel#loop with break rather than begin/end/until(or while).

and this:
#319 (comment)

Style/ParallelAssignment: Do not use parallel assignment.

To solve those, we have to apply those changes in this PR, isn't it?

[dentarg]

The Hound is always marked as green, even when it has complaints. If the lint changes were done in one PR, and we merged that, and then made another PR with the code move/protected change, the lint bot would hopefully not yell on that PR. I would prefer that.

[Mifrill]

@dentarg are we happy with those changes now after the split?