Defect #25563

Remove is_binary_data? from String

Added by Dmitry Lisichkin about 1 month ago. Updated 13 days ago.

Status:Needs feedbackStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Code cleanup/refactoring
Target version:3.5.0
Resolution: Affected version:

Description

Method is_binary_data? was removed from String class in ruby 1.9.2
https://apidock.com/ruby/v1_9_1_378/String/is_binary_data%3F

Then it was returned to String by redmine in 'rails 3.2' branch (r9475)

It cause problem with ruby-debug-ide gem:
https://github.com/ruby-debug/ruby-debug-ide/blob/ce3dbe0a76e11f34abf60740b7c88b23a1c3b5bf/lib/ruby-debug-ide/xml_printer.rb#L355
Almost all non-ASCII strings in debugger shown as [BinaryData]

I'll prefer remove that method at all but it used by SCM system.
It is a very old code (r5204) and I don't know about it is actual for now days.

But you can just rename that method to something different (see patch)

is_binary_data.patch Magnifier (1.9 KB) Dmitry Lisichkin, 2017-04-10 10:21

issue-25563.diff Magnifier - Redmine::Scm::Adapters::ScmData.is_binary_data? (2.28 KB) Toshi MARUYAMA, 2017-05-11 09:16

issue-25563_2.diff Magnifier (2.26 KB) Dmitry Lisichkin, 2017-05-11 21:44

History

#1 Updated by Toshi MARUYAMA about 1 month ago

  • Status changed from New to Needs feedback

Dmitry Lisichkin wrote:

I'll prefer remove that method at all but it used by SCM system.
It is a very old code (r5204) and I don't know about it is actual for now days.

For example, source:tags/3.3.3/test/functional/repositories_git_controller_test.rb#L448 .

It cause problem with ruby-debug-ide gem:
https://github.com/ruby-debug/ruby-debug-ide/blob/ce3dbe0a76e11f34abf60740b7c88b23a1c3b5bf/lib/ruby-debug-ide/xml_printer.rb#L355

I don't know about ruby-debug-ide, but it seems this code can be removed if ruby-debug-ide does not support Ruby 1.8.7.
Because it comes from
https://github.com/ruby-debug/ruby-debug-ide/commit/81aa3f9b66b65f02cea84945c6109e1bbe39ad66 .

Almost all non-ASCII strings in debugger shown as [BinaryData]

I think it is expected behavior of ruby-debug-ide on Ruby 1.8.

#2 Updated by Dmitry Lisichkin about 1 month ago

Toshi MARUYAMA wrote:

I don't know about ruby-debug-ide, but it seems this code can be removed if ruby-debug-ide does not support Ruby 1.8.7.
Because it comes from
https://github.com/ruby-debug/ruby-debug-ide/commit/81aa3f9b66b65f02cea84945c6109e1bbe39ad66.

I think that ruby-debug-ide IS support old Ruby

Toshi MARUYAMA wrote:

I think it is expected behavior of ruby-debug-ide on Ruby 1.8.

And this is not a problem becouse there is not so much people that work with this fossil=)

Problem in redmine code
I only say that the monkey-patching of core Ruby classes is not a good idea.
I spent some time yesterday and found that is_binary_data? method was removed only in 2.0.0 with replacement syck-engine to psych-engine
https://github.com/ruby/ruby/blob/ruby_1_9_3/ext/syck/lib/syck/rubytypes.rb#L152
https://github.com/ruby/ruby/commit/5571c7315e118b339c6b6590e666dfda68a7327d

In psych that method was moved from String to service class (the only right way):
https://github.com/ruby/psych/commit/c9cd187d5aa8fa6607dd463b5f98a65483ae39ce

So.. There is a strange thing. Redmine retrieve method to String from old YAML-engine (sic!). It's not normal.
I don't know, maybe that should be self-method in Redmine::Scm::Adapters::AbstractAdapter or in some new class or module:

def binary_data?(string)
  (string.count("^-~", "^\r\n").fdiv(string.size) > 0.3 || string.index("\x00")) unless string.empty?
end


But it definitely should not be in String.

#3 Updated by Toshi MARUYAMA 14 days ago

Dmitry Lisichkin wrote:

I don't know, maybe that should be self-method in Redmine::Scm::Adapters::AbstractAdapter or in some new class or module:

This is patch in this way.

#4 Updated by Dmitry Lisichkin 13 days ago

Toshi MARUYAMA wrote:

Dmitry Lisichkin wrote:

I don't know, maybe that should be self-method in Redmine::Scm::Adapters::AbstractAdapter or in some new class or module:

This is patch in this way.

Big thanks.
One thing: ScmData is not look as class when it have only one self-method. I think that should be a module.
And if we have ScmData constant then we can name this method just binary?

#5 Updated by Dmitry Lisichkin 13 days ago

Also available in: Atom PDF