Project

General

Profile

Actions

Feature #19261

open

`Data#members` is not important

Added by ko1 (Koichi Sasada) about 1 year ago. Updated about 1 year ago.

Status:
Open
Assignee:
-
Target version:
-
[ruby-core:111434]

Description

Data#members is defined but it is calculated by self.class.members (in other words, #members is a shorthand for self.class.members).
So it is better to remove this method.

P = Data.define(:x, :y)
p P.new(1, 2).members #=> [:x, :y]

Group = Data.define(:name, :members)
gs = Group.new('SasadaFamily', %w(ko1 yuki))
p gs.members #=> ["ko1", "yuki"]

Files

data-members-remove-method.patch (1.87 KB) data-members-remove-method.patch fabiormoura (Fabio Moura Maia), 01/08/2023 06:18 PM
data-and-struct-remove-members-method.patch (3.5 KB) data-and-struct-remove-members-method.patch fabiormoura (Fabio Moura Maia), 01/10/2023 01:22 AM

Updated by fabiormoura (Fabio Moura Maia) about 1 year ago

I've attached a patch with the changes to remove the aforementioned method. Also, I run the test suite in test_data.rb to validate my changes but there was one test failure that I'm unable to explain. However, the test failure looks to be unrelated:

% bin/ruby ../ruby/test/ruby/test_data.rb
Ignoring debug-1.7.1 because its extensions are not built. Try: gem pristine debug --version 1.7.1
Ignoring rbs-2.8.3 because its extensions are not built. Try: gem pristine rbs --version 2.8.3
Loaded suite ../ruby/test/ruby/test_data
Started
.E
==============================================================================================================================================================================================================================================================
Error: test_define_edge_cases(TestData): NoMethodError: undefined method `<=' for /duplicate member/:Regexp
../ruby/test/ruby/test_data.rb:44:in `test_define_edge_cases'
     41:     assert_same(x, o.b!)
     42:
     43:     assert_raise(ArgumentError) { Data.define(:x=) }
  => 44:     assert_raise(ArgumentError, /duplicate member/) { Data.define(:x, :x) }
     45:   end
     46:
     47:   def test_define_with_block
==============================================================================================================================================================================================================================================================
...........

I've not been able to reproduce the error with this simple ad-hoc script:

% bin/ruby -e "Data.define(:x, :x)"
-e:1:in `define': duplicate member: x (ArgumentError)

Data.define(:x, :x)
            ^^^^^^
	from -e:1:in `<main>'
% bin/ruby -e "Data.define(:x, :x)"
-e:1:in `define': duplicate member: x (ArgumentError)

Data.define(:x, :x)

Updated by k0kubun (Takashi Kokubun) about 1 year ago

If we were to remove this from Data instances, I'd like Struct instances to not have that method either for consistency. Let's say you replace Struct.new with Data.define for something that's already immutable, I don't want that change to break anything. But Data randomly lacking some shorthand methods that exist for Struct would make such refactoring harder.

Updated by fabiormoura (Fabio Moura Maia) about 1 year ago

k0kubun (Takashi Kokubun) wrote in #note-2:

If we were to remove this from Data instances, I'd like Struct instances to not have that method either for consistency. Let's say you replace Struct.new with Data.define for something that's already immutable, I don't want that change to break anything. But Data randomly lacking some shorthand methods that exist for Struct would make such refactoring harder.

I'm a newbie at ruby's codebase, but this sounds a reasonable suggestion so, I've attached a revised patch with the requested modifications. Both test suites for Data (test_data.rb) and Struct (test_struct.rb) passed.

build % make test-all TESTS=ruby/test_struct.rb
Run options:
  --seed=7424
  "--ruby=./miniruby -I../ruby/lib -I. -I.ext/common  ../ruby/tool/runruby.rb --extout=.ext  -- --disable-gems"
  --excludes-dir=../ruby/test/excludes
  --name=!/memory_leak/

# Running tests:

Finished tests in 0.037040s, 2591.7927 tests/s, 14848.8121 assertions/s.
96 tests, 550 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 3.3.0dev (2023-01-08T15:02:29Z master 8f6a9ad35d) [x86_64-darwin21]
build % make test-all TESTS=ruby/test_data.rb
Run options:
  --seed=63192
  "--ruby=./miniruby -I../ruby/lib -I. -I.ext/common  ../ruby/tool/runruby.rb --extout=.ext  -- --disable-gems"
  --excludes-dir=../ruby/test/excludes
  --name=!/memory_leak/

# Running tests:

Finished tests in 0.008879s, 1464.1288 tests/s, 12726.6584 assertions/s.
13 tests, 113 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 3.3.0dev (2023-01-08T15:02:29Z master 8f6a9ad35d) [x86_64-darwin21]

Updated by zverok (Victor Shepelev) about 1 year ago

But Data randomly lacking some shorthand methods that exist for Struct would make such refactoring harder.

Yes, that was the only reason I preserved the method in Data. Don't recall using it myself, but don't see harm in its existence either.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0