Project

General

Profile

Actions

Defect #25563

closed

Remove is_binary_data? from String

Added by Dmitry Lisichkin over 7 years ago. Updated over 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
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)


Files

is_binary_data.patch (1.9 KB) is_binary_data.patch Dmitry Lisichkin, 2017-04-10 10:21
issue-25563.diff (2.28 KB) issue-25563.diff Redmine::Scm::Adapters::ScmData.is_binary_data? Toshi MARUYAMA, 2017-05-11 09:16
issue-25563_2.diff (2.26 KB) issue-25563_2.diff Dmitry Lisichkin, 2017-05-11 21:44
Actions #1

Updated by Toshi MARUYAMA over 7 years 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.

Actions #2

Updated by Dmitry Lisichkin over 7 years 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.

Actions #3

Updated by Toshi MARUYAMA over 7 years 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.

Actions #4

Updated by Dmitry Lisichkin over 7 years 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?

Actions #6

Updated by Toshi MARUYAMA over 7 years ago

  • Target version changed from 4.1.0 to 3.4.0
Actions #7

Updated by Toshi MARUYAMA over 7 years ago

  • Status changed from Needs feedback to Closed
  • Resolution set to Fixed

Committed note-5 patch, thanks.

Actions

Also available in: Atom PDF