Refactoring array formatter

[waldyr]

Trying to move this beast from D to A. Any help, consideration or critic is welcome.

@gerrywastaken @michaeldv What do you think? I'm going to hold this until I can safely benchmark.

TODO:

• [x] Complex method #multiline_array.
• [x] Complex method #tuples.
• [ ] Limit array while processing.

  Considerations:

• #array _prefix and #tuple_prefix is almost the same method except for one space.
• limiting an array could be way better if we limit while processing.
• #generate_printable_array and #generate_printable_tuples is almost the same thing except for the prefix. The rest can be yielded.

[gerrywastaken]

Are these changes coming from commits in mg_v2 or your own changes? I'm trying to follow along, but you work fast. :)

[waldyr]

These are mine. I'm trying to improve the Code Clime score.

[gerrywastaken]

Considering the contribution guide mentions using the Ruby style guide and Rubocop does checks against this guide's recommendations (and can even --auto-correct some issues), have you considered trying it?

I found quite a few things, (problems that existed long before your commits) using it on the array_formatter.rb file.

[gerrywastaken]

BTW, not suggesting you try to fix them all in this one PR, as that might make following the changes more complicated. I'm just saying that it might be a helpful tool.

[gerrywastaken]

Reek (https://github.com/troessner/reek) is also pretty fantastic, but it tackles things which are occasionally a lot more difficult to fix, subjective, or even stuff where "fixing" it would mean you end up with worse code.

[waldyr]

@gerrywastaken I've updated this. Please take a look into the considerations section. Those further improvements may change AP output.

[gerrywastaken]

I noticed you got rid of a few comments from the methods_array method. I think it might be a good idea to keep some of the ones that explain why code is the way it. While it's often easy to figure out what a piece of code does without comments, figuring out why something was done in a certain way can sometimes be an impossible task without comments explaining it.

Personally, while the wording might not be the best, I feel the following comments give some insight into what the developer was originally thinking and I believe they mostly still apply after your changes:

# Ignore garbage, ex. 42.methods << [ :blah ]

# Avoid potential ArgumentError if object #method is overridden.

# Is this an unbound method?
# Rescue to avoid NameError when the method is not available (ex. File.lchmod on Ubuntu 12).

What is your perspective?

[waldyr]

I noticed you got rid of a few comments from the methods_array method. [...] What is your perspective?

I will put them back with a new wording 👍

[michaeldv]

@waldyr Great job so far. I am going to review your code and post my comments soon.

[waldyr]

@gerrywastaken @michaeldv Could you take a look at the #array _prefix and #tuple_prefix. They really look alike. Any chance of merging them?

[gerrywastaken]

@waldyr

def array_prefix(iteration, width)
  generic_prefix(iteration, width)
end

def tuple_prefix(iteration, width)
  generic_prefix(iteration, width, ' ')
end

def generic_prefix(iteration, width, padding=''){
  if options[:index]
    indent + colorize("[#{iteration.to_s.rjust(width)}] ", :array)
  else
    indent + padding
  end
end

How about that?

[gerrywastaken]

@waldyr I've pulled in your changes to a rebased PR https://github.com/awesome-print/awesome_print/pull/284 as I really like them an wish to merge soon.

[waldyr]

Thanks @gerrywastaken. I'm really sorry for not being responsive. My plate is beyond full right now and that's why I'm totally off.

[gerrywastaken]

@waldyr No need for apologies mate, I know exactly what it's like. I'm happy to hear you are keeping busy. :)

[gerrywastaken]

@waldyr OK, just merged your work so I'll close this PR.

I created a separate spike for your point about limiting at a different point in the code. I'm not 100% sure where you were saying that we should be limiting as the word "processing" is slightly ambiguous, but I'll figure it out when I have time to look into it. https://github.com/awesome-print/awesome_print/issues/286

Thanks again for your awesome work on this! It's greatly appreciated. :)

[waldyr]

I'm not 100% sure where you were saying that we should be limiting as the word "processing" is slightly ambiguous, but I'll figure it out when I have time to look into it.

I can surely help with that @gerrywastaken

We have a option called limit. But we only limit after we pass the array into the formatters. The order should be inverted so we can pass tiny arrays to the formatters decreasing the number of elements.

In other words we should invert those. This limiting should happen before the tuple generation

Tell me if I wasn't clear enough and I will find another way to explain it :)

[gerrywastaken]

@waldyr Oh wow, thank you! Yes that makes complete sense and I hadn't spotted that issue. Very good point!