Defect #25563
closedRemove is_binary_data? from String
0%
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
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.
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.
Updated by Toshi MARUYAMA over 7 years ago
- File issue-25563.diff issue-25563.diff added
- Target version set to 4.1.0
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.
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?
Updated by Dmitry Lisichkin over 7 years ago
- File issue-25563_2.diff issue-25563_2.diff added
Updated by Toshi MARUYAMA over 7 years ago
- Target version changed from 4.1.0 to 3.4.0
Updated by Toshi MARUYAMA over 7 years ago
- Status changed from Needs feedback to Closed
- Resolution set to Fixed
Committed note-5 patch, thanks.