New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

awesome print 2.0 #350

Open
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@imajes
Copy link
Collaborator

imajes commented Jan 22, 2019

New approach for awesomeprint -- this uses a factory pattern to register formatters as they go, etc.

feedback welcome!

imajes added some commits Jan 22, 2019

remove extraneous formatters we don't want to continue supporting (ri…
…pple, nobrainer) and relo external formatters to formatters/ext
hack to support finding AR::B classes
sigh. this needs to be done way better.
Remove mongomapper support.
it's out of date, and I don't have time to migrate and validate it.

Ideally, these 3rd party lib formatters should be added by that lib,
rather than awesome_print introspecting into the lib.

@imajes imajes requested review from maurogeorge and waldyr Jan 23, 2019

@imajes

This comment has been minimized.

Copy link
Collaborator Author

imajes commented Jan 23, 2019

@base10 this is the PR i'm talking about :D thx

@@ -6,6 +6,8 @@ module Colorize
# Pick the color and apply it to the given string as necessary.
#------------------------------------------------------------------------------
def colorize(str, type)
# puts "[COLORIZING] - using #{options[:color][type]} for #{type}" if AwesomePrint.debug

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Forgot some commented code here?

end

# Main entry point to format an object.
# register a new formatter..

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Nit: extra period

# if that's not working, lets try discover the format via formattable?
self.class.registered_formatters.each do |fmt|
puts "[FIND] trying to use [#{fmt.first.to_s.greenish} - #{fmt.last.to_s.blue}] - core: #{fmt.last.core?}" if AwesomePrint.debug
#{fmt.last.core?}" if AwesomePrint.debug

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

commented code?


module AwesomePrint
module Formatters
class BigdecimalFormatter < BaseFormatter

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Curious, should this retain the capitalization of BigDecimal?

This comment has been minimized.

@base10

base10 Jan 25, 2019

+1 on retaining BigDecimal capitalization.

This comment has been minimized.

@gokul-infrrd
]
]
]
EOS

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

If you use tilde heredocs you can preserve indentation which'll make this a bit easier to read and modify later:

http://www.virtuouscode.com/2016/01/06/about-the-ruby-squiggly-heredoc-syntax/

it 'plain multiline' do
  expect(@arr.ai(plain: true)).to eq <<~EOS.strip
    [
        [0] 1,
        [1] :two,
        [2] "three",
        [3] [
            [0] nil,
            [1] [
                [0] true,
                [1] false
            ]
        ]
    ]
  EOS
end
end

def format(object)
data = object.db_schema.inject({}) { |h, (prop, defn)| h.merge(prop => defn[:db_type]) }

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

With isolated state like inject or reduce it may make more sense to use merge! to avoid extra object allocations

require 'awesome_print/version'

module AwesomePrint
class << self

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

You can also use extend self here instead of the singleton class trick.

require 'awesome_print/ext/action_view'
# Class accessor to force colorized output (ex. forked subprocess where TERM
# might be dumb).
#---------------------------------------------------------------------------

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Probably way overkill for this PR, but may make sense to look into YARDoc later as a standard documentation framework.

def format(object)
@array = object

if object.length.zero?

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Could use object.empty? here instead.

end
hash
end
end

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Might consider breaking this out into a method, as it's getting a bit long. Perhaps something like this?

def format(object)
  if @options[:raw]
    return Formatters::ObjectFormatter.new(@inspector).format(object)
  end

  if !defined?(::ActiveSupport::OrderedHash)
    return Formatters::SimpleFormatter.new(@inspector).format(object.inspect)
  end

  object_dump = object.marshal_dump.first

  data = if object_dump.class.column_names != object_dump.attributes.keys
    object_dump.attributes
  else
    extract_column_values(object_dump)
  end

  data.merge!(details: object.details, messages: object.messages)

  "#{object} " << Formatters::HashFormatter.new(@inspector).format(data)
end

def extract_column_values(object_dump)
  object_dump
    .class
    .column_names
    .each_with_object(::ActiveSupport::OrderedHash.new) do |column_name, hash|
      if object_dump.has_attribute?(name) || object_dump.new_record?
        value = object_dump.respond_to?(name) ? object_dump.send(name) : object_dump.read_attribute(name)
        hash[name.to_sym] = value
      end
    end
end

This comment has been minimized.

@base10

base10 Jan 25, 2019

I like @baweaver's method extraction and think I'd go a step further of extracting:

data = if object_dump.class.column_names != object_dump.attributes.keys
    object_dump.attributes
  else
    extract_column_values(object_dump)
  end

to its own method, returning data.

hash[name.to_sym] = value
end
hash
end

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

May be worth considering an ActiveRecordHelper type module full of some of these types of methods, as they're repeated in a few places.

end

def format_as_instance(object)
data = (object.attributes || {}).sort_by { |key| key }.inject(::ActiveSupport::OrderedHash.new) do |hash, c|

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Could use sort instead of sort_by here

data = (object.attributes || {}).sort_by { |key| key }.inject(::ActiveSupport::OrderedHash.new) do |hash, c|
hash[c[0].to_sym] = c[1]
hash
end

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

ActiveSupport has symbolize_keys which you might find handy for this type of thing: https://apidock.com/rails/ActiveSupport/CoreExtensions/Hash/Keys/symbolize_keys

xml.gsub!(/>([^<]+)</) do |contents|
contents = colorize($1, :trueclass) if contents && !contents.empty?
">#{contents}<"
end

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

May be worth considering a Nokogiri helper for some of these features.

Depending on how much you want to look into this code it may also make sense to transfer the regex into named and documented constants to make it clearer what this code does.

end
else # Prior to Ruby 1.9 the order of set values is unpredicatble.
it 'plain multiline' do
expect(@set.sort_by { |x| x.to_s }.ai(plain: true)).to eq(@arr.sort_by { |x| x.to_s }.ai(plain: true))

This comment has been minimized.

@baweaver

baweaver Jan 24, 2019

Could use sort_by(&:to_s) to save a few characters

@baweaver

This comment has been minimized.

Copy link

baweaver commented Jan 24, 2019

Let me know if you'd be interested in me going over anything in more detail, just a few suggestions and ideas for now.

# simple formatter for now
Formatters::ObjectFormatter.new(@inspector).format(object)
end

This comment has been minimized.

@base10

base10 Jan 25, 2019

Extraneous spaces here?


"#{object} " << Formatters::HashFormatter.new(@inspector).format(data)
end

This comment has been minimized.

@base10

base10 Jan 25, 2019

Extraneous space between block ends here?

"< #{colorize(object.superclass.to_s, :class)}",
Formatters::HashFormatter.new(@inspector).format(data)
].join(' ')

This comment has been minimized.

@base10

base10 Jan 25, 2019

Extraneous new line?

].join(' ')

end

This comment has been minimized.

@base10

base10 Jan 25, 2019

Extraneous new line? (I see a lot of these below, too. If it's intentional 👍)

end

def format
# INSTANCE METHODS BELOW

This comment has been minimized.

@base10

base10 Jan 25, 2019

Curious why the comment callout here and not with the other formatters.

#------------------------------------------------------------------------------
def should_be_limited?
options[:limit] or (options[:limit].is_a?(Integer) and options[:limit] > 0)
def self.formattable?(obj)

This comment has been minimized.

@gokul-infrrd

gokul-infrrd Feb 11, 2019

Why this obj argument?

end

def format(object)

This comment has been minimized.

@gokul-infrrd

gokul-infrrd Feb 11, 2019

Is this argument required?


formatter_for :self

def self.formattable?(object)

This comment has been minimized.

@gokul-infrrd

gokul-infrrd Feb 11, 2019

Why this object argument?

#------------------------------------------------------------------------------
# FIXME: this could be super fixed.
#
def convert_to_hash(object)

This comment has been minimized.

@gokul-infrrd

gokul-infrrd Feb 11, 2019

Nice to have like this:-

Suggested change Beta
def convert_to_hash(object)
def convert_to_hash(object)
 def convert_to_hash(object)
    return nil if !object.respond_to?(:to_hash) || object.method(:to_hash).arity != 0

    hash = object.to_hash
    return nil if !hash.respond_to?(:keys) || !hash.respond_to?('[]')

    hash
  end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment