search

awesome-print / awesome_print

Pretty print your Ruby objects with style -- in full color and with proper indentation
http://github.com/michaeldv/awesome_print
MIT License
4.07k stars 454 forks source link

Refactoring - Add formatters #237

Closed waldyr closed 8 years ago

waldyr commented 8 years ago

I proposed an enhancement taking almost all methods from the God class Formatter and splitting them among other Formatters classes. Well, here it is and the code speak for itself.

I've extracted methods into classes and specifically colorize went to a module due needs from Formatter and Formatters::BaseFormatter. Any improvement or comment will be properly addressed specially if it helps me solve my todo quickly. What do you think @gerrywastaken ?

TODO

gerrywastaken commented 8 years ago

@waldyr Wow! That was very quick and impressive. 👍 I've only briefly looked at this as I've spent the entire day moving house and unfortunately had very little sleep. I'll have a better look tomorrow with fresh eyes.

Oh just a side note, when I was talking about breaking it down I was thinking along the lines of

^ That's all I was meaning, separating different types of changes into different commits. If you are applying the same transformation to many different parts of the code there there is no problem with grouping it into one commit. What I'm saying is, don't bother splitting each class into it's own commit, unless this makes your life easier. I'm more concerned with commits being task type specific and not doing a whole bunch of unrelated tasks in one commit.

It looks great though and easy to understand, I'm not complaining just clarifying my earlier request.

gerrywastaken commented 8 years ago

edit: Disregard this, I wasn't looking closely enough. It now looks like none of the public apis have changed, is that correct?

Tried to respond too quickly, my bad.

Ok so I'm thinking, with a lot of these public methods which were moved to other classes, maybe it would be a good idea to keep the old method as a simple alias to the new method and add a deprecation message to them.

According to rubygems there were 11.3 million downloads of awesome_print and 3.1 million just for the last release. I've also noticed a lot of issues raised in other gems caused by bugs (recently fixed but not yet pushed to rubygems) which were caused by their inclusion of awesome_print as a dependency. I believe it's likely that there are many instances where people have used some of our methods directly.

I think that if we were to go the "alias with deprecation" route, we might save a lot of people a lot of pain. It would also help people reading the change so they can easily see where things were moved. What are your thoughts?

Btw, feel free to amend comments and force a push on your branch. There is no expectation for it to be fixed until it's merged.

waldyr commented 8 years ago

@waldyr Wow! That was very quick and impressive.

Thanks for the cheers, man. I think refactoring is the best part of Programming and you really seem to care about the library. That was enough reasons to do it. See, you also have a part on it hahaha 😄

don't bother splitting each class into it's own commit, unless this makes your life easier

In fact I was abusing the tests to estimate how much work would be created by each step. I wanted to make sure that if I break it I would also be able to fix it later.

It now looks like none of the public apis have changed, is that correct?

Correct. Nothing has changed in terms of interfaces. I've just changed the internal behavior of Formatter. Now Formatter delegates to one of the Specific Formatters instead of trying to solve by itself.

waldyr commented 8 years ago

@gerrywastaken Finally I was able to fix the indentation problem... Pheeew 😅

options does not bother me anymore. I wish I could remove that options[:raw] from Formatter but that's a derisory thing.

Besides I want to know some things before making this ready for merge:

gerrywastaken commented 8 years ago

@waldyr I'm super sorry for taking so long to get back to you. Nobody told me I was going to be spending the last week building a truck load of IKEA furniture, which ate into all my free time. :-/

I think refactoring is the best part of Programming

Well I'm glad one of us enjoys it. I do like the end result though. :)

and you really seem to care about the library. That was enough reasons to do it.

Thanks that makes me happy to hear. :) It makes me sad when I see so many fantastic gems that have been essentially abandoned. It's nobody's fault, but it is something I want to do my part to address.

but that's a derisory thing.

I just learnt a new word. Thanks!

What do you think about Colorize being a module? Do you prefer an object being used by the formatters or it's okay as a module?

I'm just glad you separated it out. I'm showing my ignorance here, but why would somebody have a preference about this?

What do you think about the change in the spec? Do you prefer the old behavior when more entries were printed before collapsing?

OK two things, firstly I believe the failing tests were pointing to a bug.

# before this PR
def get_limit_size
  @options[:limit] == true ? DEFAULT_LIMIT_SIZE : @options[:limit]
end

# in this PR
def get_limit_size
  @options[:limit] ? DEFAULT_LIMIT_SIZE : @options[:limit]
end

As far as I can make out, the original code wasn't actually checking for a truthy value, but instead checking for it being a boolean true. If the limit is set to true, then the default limit is used, otherwise the limit itself is used. However in the change if you supply a limit the code ends up using the default limit instead, which I don't believe is the desired result.

Changing back to the old version of that method results in the tests passing without that last commit.

I raised one eyebrow when I saw the original code for get_limit_size and I can see why you made that change. While that old version seems to be correct, I think it could really use a comment to explain the logic.

waldyr commented 8 years ago

I'm super sorry for taking so long to get back to you. Nobody told me I was going to be spending the last week building a truck load of IKEA furniture, which ate into all my free time

No worries. I hope you have settled everything up by now. Moving is too stressful.

I'm just glad you separated it out. I'm showing my ignorance here, but why would somebody have a preference about this?

The problem I can see in colorize is that options and inspector isn't clearly being required. So someone who use the colorize in someplace may not define those methods inspector and options and have problems later.

I raised one eyebrow when I saw the original code for get_limit_size and I can see why you made that change. While that old version seems to be correct, I think it could really use a comment to explain the logic.

I tried to improve the affordance of the method so anyone who look at it could understand it and don't make the same mistake.

@gerrywastaken Feel free to merge it if you find convenient.

gerrywastaken commented 8 years ago

No worries. I hope you have settled everything up by now. Moving is too stressful.

Cheers. Yup I prefer to avoid it.

The problem I can see in colorize is that options and inspector isn't clearly being required. So someone who use the colorize in someplace may not define those methods inspector and options and have problems later.

Oh I see what you mean. Well it's still better than what we had. Finding a way of pulling out those dependencies can be a task for another day.

I tried to improve the affordance of the method so anyone who look at it could understand it and don't make the same mistake.

I like that. It does make things a lot clearer. Thanks for all your hard work on this PR. Merging now. 👍

gerrywastaken commented 8 years ago

For anybody reading along, these changes went live in version 1.7.0. A huge thanks to @waldyr who has since been added as a member.