[ruby-dev:50190] Re: glass:r59428 (trunk): csv.rb: use keyword parameters

[ruby-dev:50190] Re: glass:r59428 (trunk): csv.rb: use keyword parameters

[Kazuhiro NISHIYAMA]

西山和広です。

On Thu, 27 Jul 2017 18:53:58 +0900,
glass@ruby-lang.org wrote:
> 
> glass	2017-07-27 18:53:58 +0900 (Thu, 27 Jul 2017)
> 
>   New Revision: 59428
> 
>   https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=59428
> 
>   Log:
>     csv.rb: use keyword parameters
>     
>     * lib/csv.rb: usb keyword parameters to receive options
>     
>     * test/csv/test_features.rb: remove a test for checking options
> 
>   Modified files:
>     trunk/lib/csv.rb
>     trunk/test/csv/test_features.rb

の差分をみて気になった点を送っておきます。

>    # :call-seq:
> -  #   filter( options = Hash.new ) { |row| ... }
> -  #   filter( input, options = Hash.new ) { |row| ... }
> -  #   filter( input, output, options = Hash.new ) { |row| ... }
> +  #   filter( **options ) { |row| ... }
> +  #   filter( input, **options ) { |row| ... }
> +  #   filter( input, output, **options ) { |row| ... }

とあるのに、

> -  def self.filter(*args)
> +  def self.filter(input, output=nil, **options)

となっていて input が必須引数になってしまっています。

%  ruby -vr csv -e 'CSV.filter' < /dev/null
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
%  ruby -vr csv -e 'CSV.filter' < /dev/null
ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
Traceback (most recent call last):
	1: from -e:1:in `<main>'
/Users/kazu/.rbenv/versions/git/lib/ruby/2.5.0/csv.rb:1102:in `filter': wrong number of arguments (given 0, expected 1..2) (ArgumentError)

> -  def self.generate_line(row, options = Hash.new)
> -    options  = {row_sep: $INPUT_RECORD_SEPARATOR}.merge(options)
> -    encoding = options.delete(:encoding)
> -    str      = String.new
> +  def self.generate_line(row, encoding: nil, **options)
> +    options[:row_sep] ||= $INPUT_RECORD_SEPARATOR
> +    str = String.new

row_sep が偽の時に無視されるようになったようですが、意図的でしょうか?

%  ruby -vr csv -e 'p CSV.generate_line(%w"1 2", row_sep: nil)'
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
"1,2"
%  ruby -vr csv -e 'p CSV.generate_line(%w"1 2", row_sep: nil)'
ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
"1,2\n"

> -  def self.open(*args)
> -    # find the +options+ Hash
> -    options = if args.last.is_a? Hash then args.pop else Hash.new end
> +  def self.open(*args, **options)
>      # wrap a File opened with the remaining +args+ with no newline
>      # decorator
> -    file_opts = {universal_newline: false}.merge(options)
> +    file_opts = options.dup
> +    file_opts[:universal_newline] ||= false

open の universal_newline が通るようになったようですが、
initialize で Unknown options にならなくなったのは意図的でしょうか?
(テストの方で test_unknown_options を消しているので意図的っぽい?)

%  echo -en 'foo,bar\r\n1,2\n' > /tmp/foo.csv
%  ruby -vr csv -e 'p CSV.open("/tmp/foo.csv", universal_newline: true, &:read)'
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
/Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1548:in `initialize': Unknown options:  universal_newline. (ArgumentError)
	from /Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1273:in `new'
	from /Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1273:in `open'
	from -e:1:in `<main>'
%  ruby -vr csv -e 'p CSV.open("/tmp/foo.csv", universal_newline: true, &:read)'
ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
[["foo", "bar"], ["1", "2"]]
%  ruby -vr csv -e 'p CSV.new("/tmp/foo.csv", universal_newline: true)'
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
/Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1548:in `initialize': Unknown options:  universal_newline. (ArgumentError)
	from -e:1:in `new'
	from -e:1:in `<main>'
%  ruby -vr csv -e 'p CSV.new("/tmp/foo.csv", universal_newline: true)'
ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
<#CSV io_type:StringIO encoding:UTF-8 lineno:0 col_sep:"," row_sep:"\n" quote_char:"\"">

> -      file_opts = {encoding: Encoding.default_external}.merge(file_opts)
> +      file_opts[:encoding] ||= Encoding.default_external

ここも偽の時に非互換が発生しそうですが、意図的でしょうか?

> +      encoding, = encoding.split(":") if encoding.is_a?(String)

split(":", 2) にすると良さそうです。


-- 
|ZnZ(ゼット エヌ ゼット)
|西山和広(Kazuhiro NISHIYAMA)

[ruby-dev:50191] Re: glass:r59428 (trunk): csv.rb: use keyword parameters

[Masaki Matsushita]

松下 (glass)です。

コメント頂きありがとうございます。
r59437でご指摘頂いた点を修正して、いくつかテストを追加しました。
Unknown optionsについてはキーワード引数として検査するようにしています。

松下

2017-07-27 23:30 GMT+09:00 Kazuhiro NISHIYAMA <zn@mbf•nifty.com>:
> 西山和広です。
>
> On Thu, 27 Jul 2017 18:53:58 +0900,
> glass@ruby-lang.org wrote:
>>
>> glass 2017-07-27 18:53:58 +0900 (Thu, 27 Jul 2017)
>>
>>   New Revision: 59428
>>
>>   https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=59428
>>
>>   Log:
>>     csv.rb: use keyword parameters
>>
>>     * lib/csv.rb: usb keyword parameters to receive options
>>
>>     * test/csv/test_features.rb: remove a test for checking options
>>
>>   Modified files:
>>     trunk/lib/csv.rb
>>     trunk/test/csv/test_features.rb
>
> の差分をみて気になった点を送っておきます。
>
>>    # :call-seq:
>> -  #   filter( options = Hash.new ) { |row| ... }
>> -  #   filter( input, options = Hash.new ) { |row| ... }
>> -  #   filter( input, output, options = Hash.new ) { |row| ... }
>> +  #   filter( **options ) { |row| ... }
>> +  #   filter( input, **options ) { |row| ... }
>> +  #   filter( input, output, **options ) { |row| ... }
>
> とあるのに、
>
>> -  def self.filter(*args)
>> +  def self.filter(input, output=nil, **options)
>
> となっていて input が必須引数になってしまっています。
>
> %  ruby -vr csv -e 'CSV.filter' < /dev/null
> ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
> %  ruby -vr csv -e 'CSV.filter' < /dev/null
> ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
> Traceback (most recent call last):
>         1: from -e:1:in `<main>'
> /Users/kazu/.rbenv/versions/git/lib/ruby/2.5.0/csv.rb:1102:in `filter': wrong number of arguments (given 0, expected 1..2) (ArgumentError)
>
>> -  def self.generate_line(row, options = Hash.new)
>> -    options  = {row_sep: $INPUT_RECORD_SEPARATOR}.merge(options)
>> -    encoding = options.delete(:encoding)
>> -    str      = String.new
>> +  def self.generate_line(row, encoding: nil, **options)
>> +    options[:row_sep] ||= $INPUT_RECORD_SEPARATOR
>> +    str = String.new
>
> row_sep が偽の時に無視されるようになったようですが、意図的でしょうか?
>
> %  ruby -vr csv -e 'p CSV.generate_line(%w"1 2", row_sep: nil)'
> ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
> "1,2"
> %  ruby -vr csv -e 'p CSV.generate_line(%w"1 2", row_sep: nil)'
> ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
> "1,2\n"
>
>> -  def self.open(*args)
>> -    # find the +options+ Hash
>> -    options = if args.last.is_a? Hash then args.pop else Hash.new end
>> +  def self.open(*args, **options)
>>      # wrap a File opened with the remaining +args+ with no newline
>>      # decorator
>> -    file_opts = {universal_newline: false}.merge(options)
>> +    file_opts = options.dup
>> +    file_opts[:universal_newline] ||= false
>
> open の universal_newline が通るようになったようですが、
> initialize で Unknown options にならなくなったのは意図的でしょうか?
> (テストの方で test_unknown_options を消しているので意図的っぽい?)
>
> %  echo -en 'foo,bar\r\n1,2\n' > /tmp/foo.csv
> %  ruby -vr csv -e 'p CSV.open("/tmp/foo.csv", universal_newline: true, &:read)'
> ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
> /Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1548:in `initialize': Unknown options:  universal_newline. (ArgumentError)
>         from /Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1273:in `new'
>         from /Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1273:in `open'
>         from -e:1:in `<main>'
> %  ruby -vr csv -e 'p CSV.open("/tmp/foo.csv", universal_newline: true, &:read)'
> ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
> [["foo", "bar"], ["1", "2"]]
> %  ruby -vr csv -e 'p CSV.new("/tmp/foo.csv", universal_newline: true)'
> ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin15]
> /Users/kazu/.rbenv/versions/2.3.3/lib/ruby/2.3.0/csv.rb:1548:in `initialize': Unknown options:  universal_newline. (ArgumentError)
>         from -e:1:in `new'
>         from -e:1:in `<main>'
> %  ruby -vr csv -e 'p CSV.new("/tmp/foo.csv", universal_newline: true)'
> ruby 2.5.0dev (2017-07-27 trunk 59435) [x86_64-darwin16]
> <#CSV io_type:StringIO encoding:UTF-8 lineno:0 col_sep:"," row_sep:"\n" quote_char:"\"">
>
>> -      file_opts = {encoding: Encoding.default_external}.merge(file_opts)
>> +      file_opts[:encoding] ||= Encoding.default_external
>
> ここも偽の時に非互換が発生しそうですが、意図的でしょうか?
>
>> +      encoding, = encoding.split(":") if encoding.is_a?(String)
>
> split(":", 2) にすると良さそうです。
>
>
> --
> |ZnZ(ゼット エヌ ゼット)
> |西山和広(Kazuhiro NISHIYAMA)