Awesome Print vN.... (was v2, but not anymore..)

[maurogeorge]

Hi @michaeldv @imajes this is a big commit, with tons of refactorings and close the issues to we bump a version 2.0.

Since we have a lot of changes, I think it is better take a look on the branch at https://github.com/michaeldv/awesome_print/tree/mg-v2

To fix the #187 I create N formatters, so all new formatter can have a single place to format the object like this. Removed the monkey patch on the plugins too 🤘.

To fix the #188 I create a new option, false by default. Since the default on ruby is to use the hash rocket.

2.1.0 :001 > { foo: :bar }
 => {:foo=>:bar}

To fix the #189 I droped the Ruby 1.8.x and Rails 2.x from the code and specs.

Beside the issues, I change the structure a lot. Since in the last commits we have more code coverage, I feel more confident to make this changes.

Since this is a big step I think we can release a 2.0.0.beta first, and wait a little before release the 2.0.0 this way we can fix some possible bug on this. We can define a number of downloads on Rubygems to the beta, and after the downloads we release the final version.

Please review guys, I hope you guys enjoy. Lets work together to release a new awesome_print 🎆

@michaeldv thanks for bring the awesome_print to the world. On my first studies with Ruby/Rails I used the AP and love it. Today I work about 4 years with Ruby/Rails and use AP on my daily basis. It is so awesome I can contribute to this project 😊

closes #189 #187 #188

[maurogeorge]

@alexch thanks for the review and join the discussion!

I will remove this Base classes and follow your convention.

About the plugin structure the main changes are:

• no more monkey patching to discover what is the type of the object
• check only once the existance of a external dependency, only on the require
• A single class to each custom type.

I think you can take a look at this commit michaeldv/awesome_print@4d6e963 all the types follow this same line of thinking.

I will update to create the Formatter.from(self, object) to be more Ruby way, In this particular case I prefer use the full class and method since it is a private object used only inside the AP, but yeah, will be better the short version with a class method.

Again @alexch thanks for the review, wait for more feedback ❤️

[maurogeorge]

@michaeldv no, I never wrote a line in Java. But I like the idea of OOP and Design pattern in the Ruby World.

For example in the master version of the hash formatter we have all the logic in a single method and some helpers method like the should_be_limited? that is used only on array and hash in a single class. I extracted a specific class to handle this formatter and since now I am on a single class it is easy to apply some extract method.

This idea of a class to each is followed on rails for example on the Action View where all the tags have a single class. So basically we moved from a single method that handle a formatter to a specific class, wich is better to we apply some good practices like SRP, small classes, small methods and pass little params.

[imajes]

@maurogeorge: this is pretty epic. thanks! 👍

However... I don't think it should be one PR, and I vote not to land it. It's just too big to review as one piece; too many things going on.

Can we break out a separate PRs for:

• the readme update (we should really make clear that this is the last release for old rubies :))
• the actual removal of the old ruby support
• the copyright and other misc cleanups
• new formatter base, and then the implementations
• handle type in the same way?
• anything else?

I know it sounds like a bunch of work, but I think it'll be much easier to review, allowing all of these PRs contribute to the 2.0 milestone. :)

[maurogeorge]

@imajes thanks ❤️

I understand, in this specific case I prefer do all the work in a single PR because I have some dependencies, for example a class that I created one the earlier commits are used on the last ones. So it is not possible I made N PR in a single time.

The good thing is, since I am on a single branch, if we have some fix to do on the 1.6.0 it is easy to fix and release 1.6.1 when we start 2.0(on master) we will stop to work on 1.6.x. So my idea is to work on v2 on this branch and merge only when it is good to release, this way we will drop the 1.6.x but with a 2.0.0.beta.

I think If I create some diffs grouped by feature/fix can be better take, a look.

• Remove copyright and some old stuffs
• Extract Configuration from Inspector
• Create Formatters built in
• Create Formatters of extensions
• Create FormatterFactory and some refactorings
• Create Types classes (removes no need monkey patch)
• Simple require of files and move some responsabilities to the right place
• Format hash using the new syntax
• Define the version in a single place

I think this way is more easy to do the review, if we treat a diff like a PR. Hope thats helps 😄

Beside that @alexch will do a review too.

[michaeldv]

@maurogeorge Thanks for all your hard work. I guess we have some major differences on this one.

First, I don't want to turn the core gem into one monstrosity with all the formatters under one roof. awesome_print should provide formatters for basic Ruby types, everything else could be made available as plugins/extensions. Ideally I would move all /ext formatters into separate gems with their own tests, maintainers, etc.

Second, what's wrong with method-based awesome_#{type} dispatch? Your implementation replaces it with class-based AwesomePrint::Formatters::#{type}. What makes you think this is so much better and worth all the overhead (20+ new requires, constantize, loading and instantiating new classes, etc.)? Besides, your type discovery uses .map and goes over all custom types rather that returning first non-nil one.

Third, you seem to think removing so-called "monkey patching" is a good thing, whereas I think it lets us separate custom extensions from the core gem, and load them as necessary without maintaining explicit list of supported custom types.

Fourth, you can add your own copyright but don't remove somebody else's. You just don't.

Anyway, I am all for refactoring and making the code better, however writing more code with no apparent benefit doesn't seem like a right approach. I am always in favor of reducing the codebase, not growing it.

[imajes]

Yeah-

@michaeldv, i think that comment is exactly why this PR is too big to move on :)

It'd be nice to talk about some of these things and review approach/strategy. For example i am not entirely sure that the idea of having many separate gems is super great - one of ap's biggest wins for me is that it's basically ready to go- you don't have to bring more gems into the mix to just 'get it to work'.

definitely worth talking about -- because your point about the custom extensions makes sense too :)

[alexch]

I'm going to wait until I review the entire PR before I weigh in on the complexity debate, but I thought I'd give my 2c on this:

Fourth, you can add your own copyright but don't remove somebody else's. You just don't.

He didn't remove your copyright. He removed some superfluous comments from the source code. The code still retains your copyright via the README and LICENSE files. And it's just a PR; if you feel strongly that individual source files need a copyright line, you can make that case in a less hostile way. (Please forgive me if I'm misinterpreting your tone, but "you just don't" sounds pretty harsh, especially in light of the effort @maurogeorge put into this PR.)

Personally I agree with the points made here: http://opensource.com/law/14/2/copyright-statements-source-files -- and I'd like to know if you've got some compelling arguments to the contrary.

[alexch]

Also: http://hackerboss.com/get-rid-of-templates/

You don’t need a copyright notice in every file... You don’t need to assert copyright because of the Berne Convention for the Protection of Literary and Artistic Works. It’s an international agreement which governs copyright... The convention lays down the basics of what copyright is and what it means, and then says that “the enjoyment and the exercise of these rights shall not be subject to any formality”. This means that you don’t need to do anything to have copyright over your works. You don’t need to include a “Copyright ©” text... You can include a copyright text, but the Berne Convention says it has no legal meaning... Before the United States had signed the Berne Agreement in 1989, a copyright assertion was actually required if you wanted to enforce your copyright in the US. Everybody is still doing it, even though nobody needs to.

[maurogeorge]

@alexch I made your suggestions please take a look.

@michaeldv

First, I don't want to turn the core gem into one monstrosity with all the formatters under one roof...

This our situation today on master or this branch. Maybe is a good idea to move to external gems, like the rspec did. But IMHO this is a big step, so maybe we need to do a baby step first.

Second, what's wrong with method-based awesome_#{type} dispatch?...

Nothing! In the early days of the project probably make a lot of sense, but today we have a lot of formatters. So the code grows and we need to refactoring and thinking on a better solution. I sugested classes because it is better to we apply some good practices like SRP, small classes, small methods and pass a few params. The focus is on clarity, example if I want change the hash formatter, just open the AwesomePrint::Formatters::Hash, if my change is too complex I can split this on single methods inside the Hash formatter only.

Besides, your type discovery uses .map and goes over all custom types rather that returning first non-nil one.

Good catch! Let`s try find together a better solution to that.

Third, you seem to think removing so-called "monkey patching" is a good thing...

Why we need to load custom extensions? All the extensions today are inside the AP.

Fourth, you can add your own copyright but don't remove somebody else's. You just don't.

Sorry for that.
As @alexch said I didn't remove your copyright. I removed some superfluous comments from the source code.
I follow the convention of some big projects on the community like Rails, Devise, Paperclip etc. That do not have this comments on each file.

Anyway, I am all for refactoring and making the code better, however writing more code with no apparent benefit doesn't seem like a right approach. I am always in favor of reducing the codebase, not growing it.

Again as my answer on 2, my focus here is on clarity.

[michaeldv]

Thanks for all your feedback! Let me step back a little to provide some context. When I had first built awesome_print it only covered basic Ruby types. Very quickly I realized I want custom formatter for ActiveRecord types. So I've extended type dispatch to make the following possible:

# Load core gem.
require "awesome_print"

# Load custom formatter. As long as it follows certain rules the core gem will
# pick it up and apply the formatting to custom types automagically.
require "my_awesome_custom_object_formatter"

Back then I was using Rails on daily basis so I've added my ActiveRecord extension to the gem itself (in retrospect though I should have created separate "awesome_print_rails" gem).

Anyway, I think preserving "drop-in" mechanism without explicitly hardcoding a list of available custom formatters has a value. My goal in v2 branch was to 1) make dispatch chaining more DSL-ish and less explicit, and 2) document the interface so that people could create extensions outside of awesome_print gem.

I am willing to give this PR a benefit of a doubt and run some benchmarks (and I encourage you to do the same). However if we are throwing in more code that is slower and doesn't add new features (and in fact removes dynamic loading I've described above) then I am highly doubtful this is the right way to go.

As for the copyright, I didn't mean to sound harsh ;-) Ironically though the license itself states that the copyright notice shall be included in substantial portions of the software -- so you know who to blame ;-)

[maurogeorge]

@michaeldv I understand.

But I dont think we need this "drop in" mechanism, since today we have lot of formatters like the MongoMapper, Mongoid, Nokogiri, NoBrainer etc. All this formatters live inside the AP, so they are a private API to the AP prints this objects, I dont think AP need a Public API to handle extensions, in the moment. This way anyone wants a new formatter they can do a PR to AP and add this formatter to AP.

To handle this external's extensions, first we need to expose a set of methods that we define as a public API, doing a RDoc to this, this way all extensions know how method they can use to built a AP extension.

Beside that the actual "drop in" it is not so "drop in", since we need to check and require the formatters.

Probably the idea of have like:

• awesome_print
• awesome_print-core
• awesome_print-rails
• awesome_print-mongoid

It is a good thing. But I think it is a big step, to me this PR is a first step, before the next step of define a public API and later extract externals gems. I think we can treat the AP like the devise, devise has 10 modules to handle a specific part of authentication, but all that is bundle in a single gem.

My idea of this PR is to fixes the #189 #187 #188 and try to enhance the code quality. As you can see on Code Climate this PR enhance the GPA in +0.89 now we do not have a single F class, since now we have more classes it is more easy to refactor and reach a better quality.

About the copyright we can remove from each file right?

[maurogeorge]

@michaeldv I updated the code to preserving the "drop-in" mechanism michaeldv/awesome_print@aef581d.

[maurogeorge]

@michaeldv any feedback on that?

[michaeldv]

@maurogeorge I run some benchmarks on my MacBook Air and the results are discouraging: it's more than 15 times slower in my test. To run it I set RUBYLIB environment variable to point to local copy of the code, and then flipped between master and mg-v2 branches,

#!/usr/bin/env ruby
#
# $ export RUBYLIB=~/Source/Ruby/ap20/awesome_print/lib/

### puts $LOAD_PATH.inspect

require "awesome_print"
require "benchmark"

# AwesomePrint 1.6.2 / Ruby 2.0.0p481
#        user     system      total        real
#    4.190000   0.160000   4.350000 (  4.346654)

# AwesomePrint 1.6.2 / Ruby 2.1.3p242
#        user     system      total        real
#    4.060000   0.170000   4.230000 (  4.225158)

#--------------------------------------------------------------

# AwesomePrint 2.0.0.beta / Ruby 2.0.0p481
#       user     system      total        real
#  67.000000   5.890000  72.890000 ( 72.902481)

# AwesomePrint 2.0.0.beta / Ruby 2.1.3p242
#       user     system      total        real
#  65.020000   5.670000  70.690000 ( 70.691979)

puts "AwesomePrint #{AwesomePrint.version} / Ruby #{RUBY_VERSION}p#{RUBY_PATCHLEVEL}"

data = [ false, 42, %w(forty two), { :now => Time.now, :class => Time.now.class, :distance => 42e42 } ]
formatted = data.ai
puts formatted

Benchmark.bm do |bm|
  bm.report do
    20_000.times do
      formatted = data.ai
    end
  end
end

I suspect it's all because of class-based dispatch: you basically look up and create new class for each formatter call, and that's a lot of overhead (even more than I anticipated).

Also, your code even with custom type detection might break gems like https://github.com/estum/awesome_print-carrierwave which follow current extension loading pattern.

[maurogeorge]

@michaeldv I suspected that is slow too. But in this case I prefer code quality over speed, since the Awesome Print, I think, is used only in development/test.

Let's compare the "qualities" on two branches we have, in my opinion:

mg-v2

• Code Quality
• Support new hash syntax via option
• Droped the Ruby 1.8.x and Rails 2.x from the code and specs

master

• Faster

IMHO with a better quality I think will be more easy to start some refactorings and close some issues. Obviously be faster is always a good thing, but I don't think this will be noted when people are on a console. Maybe we can improve speed even using classes and autoload, I don't think that we need lost code quality to gain speed.

Yeah probably with this PR will break gems like the one you mentioned, but the mentioned gem do not follow a public API to make extensions, today AP do not have that. And this will be the 2.0 so we are good to break things.

Clearly we diverge in opinion here, but the final decision is yours 😃

[imajes]

I still don't understand why you are unwilling to break up this PR. Land the easy stuff first. I think this approach has been a bit irresponsible and you are kind of forcing it with the all or nothing approach.

As a committer I would have to vote no, because this just isn't good dev practice. Sorry Mauro, but this can't be the best way to do this! ☺

On Apr 21, 2015, at 06:16, Mauro George [email protected] wrote:

@michaeldv I suspected that is slow too. But in this case I prefer code quality over speed, since the Awesome Print, I think, is used only in development/test.

Let's compare the "qualities" on two branches we have, in my opinion:

mg-v2

Code Quality
Support new hash syntax via option
Droped the Ruby 1.8.x and Rails 2.x from the code and specs
master

Faster
IMHO with a better quality I think will be more easy to start some refactorings and close some issues. Obviously be faster is always a good thing, but I don't think this will be noted when people are on a console. Maybe we can improve speed even using classes and autoload, I don't think that we need lost code quality to gain speed.

Yeah probably with this PR will break gems like the one you mentioned, but the mentioned gem do not follow a public API to make extensions, today AP do not have that. And this will be the 2.0 so we are good to break things.

Clearly we diverge in opinion here, but the final decision is yours

—
Reply to this email directly or view it on GitHub.

[michaeldv]

I agree with @imajes -- if we could extract new hash syntax and then drop legacy support for older Ruby and Rails that would be great way to move forward.

I agree that many developers are using awesome_print in their local dev/test environments only. However we simply don't know who and how is using it in production. Knowingly releasing something that is so much slower feels somewhat irresponsible to me.

[maurogeorge]

@imajes I explain my decision about make a single PR about V2 on the comment.

The good thing is, since I am on a single branch, if we have some fix to do on the 1.6.0 it is easy to fix and release 1.6.1 when we start 2.0(on master) we will stop to work on 1.6.x. So my idea is to work on v2 on this branch and merge only when it is good to release, this way we will drop the 1.6.x but with a 2.0.0.beta.

@michaeldv I understand your point.

[denispeplin]

It seems still no tool (ap/pp/whatever) to print hashes with new syntax. Strange.

[BryanH]

Will break this stuff up into smaller PRs, as a lot of it is worth saving!