Don't monkey patch String class #91

n-rodriguez · 2023-04-21T16:34:04Z

As explained here awesome-print/awesome_print#419, monkey patch on String class was pretty unexpected.

The key is in def colorize(str, type) method : instead of monkey patching String + calling 'foo'.red, let's define colors methods on AmazingPrint::Colors module and call AmazingPrint::Colors.red('foo') instead.

amazing_print/lib/amazing_print/colorize.rb

Lines 9 to 22 in 8c0ae33

    
           def colorize(str, type) 
        
             str = CGI.escapeHTML(str) if options[:html] 
        
             if options[:plain] || !options[:color][type] || !inspector.colorize? 
        
               str 
        
             # 
        
             # Check if the string color method is defined by amazing_print and accepts 
        
             # html parameter or it has been overriden by some gem such as colorize. 
        
             # 
        
             elsif str.method(options[:color][type]).arity == -1 # Accepts html parameter. 
        
               str.send(options[:color][type], options[:html]) 
        
             else 
        
               str = %(<kbd style="color:#{options[:color][type]}">#{str}</kbd>) if options[:html] 
        
               str.send(options[:color][type]) 
        
             end

This way we don't pollute String class with new methods and we avoid clashes with colorize gem which I don't use due to license issues (gpl2 is contaminating) and we do not have to add a new dependency like Rainbow gem.

Thank you!

ddarbyson · 2023-04-21T18:59:05Z

nice to see some movement on this

paddor

Yeah this is long overdue. Thank you. At first glance, this looks good to me.

HarlemSquirrel

Thanks for working on this! This is very much needed.

Could you please check out the lints and make Robocop happy? 😁

n-rodriguez · 2023-04-22T20:24:57Z

Could you please check out the lints and make Robocop happy? 😁

Done :)

HarlemSquirrel

HarlemSquirrel · 2023-04-22T21:21:10Z

Those other checks appear to be failing on master so we can fix those separately.

n-rodriguez · 2023-04-22T21:22:14Z

Thank you!

This was referenced Apr 21, 2023

allow using without monkey-patches #36

Closed

Disable color for no_tty #80

Closed

paddor previously approved these changes Apr 21, 2023

View reviewed changes

HarlemSquirrel reviewed Apr 22, 2023

View reviewed changes

Don't monkey patch String class

481afdc

n-rodriguez dismissed paddor’s stale review via 481afdc April 22, 2023 20:23

Fix rubocop offenses

31c1c92

n-rodriguez requested a review from HarlemSquirrel April 22, 2023 20:26

HarlemSquirrel approved these changes Apr 22, 2023

View reviewed changes

HarlemSquirrel merged commit 26c8bf3 into amazing-print:master Apr 22, 2023

basicallydan mentioned this pull request May 26, 2023

v1.5 contains a breaking change due to the string convenience methods being removed #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't monkey patch String class #91

Don't monkey patch String class #91

n-rodriguez commented Apr 21, 2023 •

edited

Loading

ddarbyson commented Apr 21, 2023

paddor left a comment

HarlemSquirrel left a comment

n-rodriguez commented Apr 22, 2023

HarlemSquirrel left a comment

HarlemSquirrel commented Apr 22, 2023

n-rodriguez commented Apr 22, 2023

	def colorize(str, type)
	str = CGI.escapeHTML(str) if options[:html]
	if options[:plain] \|\| !options[:color][type] \|\| !inspector.colorize?
	str
	#
	# Check if the string color method is defined by amazing_print and accepts
	# html parameter or it has been overriden by some gem such as colorize.
	#
	elsif str.method(options[:color][type]).arity == -1 # Accepts html parameter.
	str.send(options[:color][type], options[:html])
	else
	str = %(<kbd style="color:#{options[:color][type]}">#{str}</kbd>) if options[:html]
	str.send(options[:color][type])
	end

Don't monkey patch String class #91

Don't monkey patch String class #91

Conversation

n-rodriguez commented Apr 21, 2023 • edited Loading

ddarbyson commented Apr 21, 2023

paddor left a comment

Choose a reason for hiding this comment

HarlemSquirrel left a comment

Choose a reason for hiding this comment

n-rodriguez commented Apr 22, 2023

HarlemSquirrel left a comment

Choose a reason for hiding this comment

HarlemSquirrel commented Apr 22, 2023

n-rodriguez commented Apr 22, 2023

n-rodriguez commented Apr 21, 2023 •

edited

Loading