Project

General

Profile

Actions

Bug #19259

closed

`Data#with` doesn't call `initialize` nor `initialize_copy`

Added by ko1 (Koichi Sasada) almost 2 years ago. Updated almost 2 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:111431]

Description

Data#with doesn't call initialize nor initialize_copy.
It is confirmation request.

class P < Data.define(:x, :y)
  def initialize_copy(...)
    p :initialize_copy
    super
  end

  def initialize(...)
    p :initialize
    super
  end
end

pt = P.new(1, 2)
#=> :initialize
pt.clone
#=> :initialize_copy
pt.dup
#=> :initialize_copy
pt.with(x: 10)
#=> N/A

For example, if an author of P add validation code, #with skips it.


Related issues 1 (0 open1 closed)

Related to Ruby master - Feature #19000: Data: Add "Copy with changes method" [Follow-on to #16122 Data: simple immutable value object]ClosedActions
Actions #1

Updated by ko1 (Koichi Sasada) almost 2 years ago

  • Description updated (diff)
Actions #2

Updated by ko1 (Koichi Sasada) almost 2 years ago

  • Description updated (diff)

Updated by matz (Yukihiro Matsumoto) almost 2 years ago

In principle, it should call initialize. But we will reconsider if any there's any concern (e.g., performance).

Matz.

Actions #4

Updated by Eregon (Benoit Daloze) almost 2 years ago

  • Related to Feature #19000: Data: Add "Copy with changes method" [Follow-on to #16122 Data: simple immutable value object] added

Updated by Eregon (Benoit Daloze) almost 2 years ago

My expectation is it would call initialize as well.
I didn't think about that in the review of https://github.com/ruby/ruby/pull/6766, sorry.
So like:

class Data
  def with(**changes)
    self.class.new(**to_h, **changes)
  end
end

BTW, should .new be called dynamically too?

BTW ruby/spec's mock/should_receive should be useful to test whether initialize is called and with which arguments.

Updated by zverok (Victor Shepelev) almost 2 years ago

BTW, should .new be called dynamically too?

Intuitively, I don't think so, but I fail to find good logical arguments :)

(We can, for example, note that with is some Data-specific and fancy copying/duplication method, and dup/clone don't invoke .new)

Updated by Eregon (Benoit Daloze) almost 2 years ago

zverok (Victor Shepelev) wrote in #note-6:

Intuitively, I don't think so, but I fail to find good logical arguments :)
(We can, for example, note that with is some Data-specific and fancy copying/duplication method, and dup/clone don't invoke .new)

Yeah, I think it's uncommon to call .new dynamically for core classes so probably no need here.
It would matter if someone defined a custom new, but our expectation with Data is that people define a custom initialize and not a custom new.

Actions #9

Updated by nobu (Nobuyoshi Nakada) almost 2 years ago

  • Status changed from Open to Closed

Applied in changeset git|45f0e3a673964069a4c9c57ce8665cbc21ac267f.


[Bug #19259] Data#with should call initialize method

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like1Like0Like0Like0Like0