Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge named tuples #2634

Closed
wants to merge 3 commits into from
Closed

Merge named tuples #2634

wants to merge 3 commits into from

Conversation

emancu
Copy link

@emancu emancu commented May 23, 2016

I think it is useful and also completes the interface with a method defined in the tuples specification.

Thanks @soveran for his smart implementation
@lbguilherme
Copy link
Contributor

@lbguilherme lbguilherme commented May 23, 2016

This is useful for default options. You merge a named tuple with the default values for some options with the user provided values for some of them.

I would prefer a merge method for this instead of +. The problem is that addition is commutative, and a merge is not. This gives the false impression that it will be commutative.

By the way, this implementation does not work in cases where the two tuples have the same key (those are the ones where commutativity is broken)

{a: 1, b: 2} + {b: 3, c: 4}  # compile error. expected:  {a: 1, b: 3, c: 4}

I don't know how to implement this.

identity(**self, **other)
end

private def identity(**options)
Copy link
Member

@jhass jhass May 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just calling NamedTuple.new instead should work as well.

Copy link
Author

@emancu emancu May 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhass I forgot that NamedTuple.new works like that. I'm fixing it

@jhass
Copy link
Member

@jhass jhass commented May 23, 2016

I'm not sure about the default values usecase, normally you would

def foo(*, a = 1, b = 2)
end

@emancu
Copy link
Author

@emancu emancu commented May 23, 2016

@lbguilherme the main intention was to compose NamedTuples and add well defined pairs and keep the immutability.

This is useful for default options. You merge a named tuple with the default values for some options with the user provided values for some of them.

As @jhass mention, you probably want to use the following snippet for default values

def foo(*, a = 1, b = 2)
end

I would prefer a merge method for this instead of +. The problem is that addition is commutative, and a merge is not. This gives the false impression that it will be commutative.

I prefer the merge method too, but I discarded that name because:

  • Tuple already defines a + and I wanted to keep the same API
  • Using merge may confuse you with the Ruby method, where you can override values or mutate the object
  • The false impression of commutative you mention

By the way, this implementation does not work in cases where the two tuples have the same key (those are the ones where commutativity is broken)

I know and that is expected. You can't define NamedTuples repeating a key neither.
I'm not trying to make NamedTuples behave as a Hash.

The reason that I was looking for a method like this is that I'm writing an ORM where the attributes are stored as a NamedTuple instead of a Hash.
I'm still working on that code, but I got stuck because the lack of this feature (or my programming skills ^^)

Here is an example of the code using this cool feature and why I want it.

def save
  @attributes = {name: "emancu", profession: "programmer"}

  id = DB.persist @attributes

  @attributes = {id: id} + @attributes
end

@asterite
Copy link
Member

@asterite asterite commented May 23, 2016

But that creates a union of named tuples, which you won't be able to splat later. I think hashes are more suitable for this use case.

@asterite
Copy link
Member

@asterite asterite commented May 23, 2016

Note that I'm not against merging named tuples, and I think it can even be done if keys repeat, I'm just not sure named tuples should be used in places where a Hash is more useful.

@emancu
Copy link
Author

@emancu emancu commented May 23, 2016

@asterite You are right, maybe that is not the best code example.

@asterite
Copy link
Member

@asterite asterite commented May 23, 2016

Here's an implementation that preserves the values on the other merged named tuple.

Code:

struct NamedTuple
  def self.new(**options)
    options
  end

  def merge(other : NamedTuple)
    merge_implementation(other)
  end

  private def merge_implementation(other : U)
    {% begin %}
      NamedTuple.new(
        {% for key in (T.keys + U.keys).uniq %}
          {% if U.keys.includes?(key) %}
            {{key}}: other[:{{key}}],
          {% else %}
            {{key}}: self[:{{key}}],
          {% end %}
        {% end %}
      )
    {% end %}
  end
end

a = {x: 1, y: 2}
b = {z: 3}

c = a.merge(b)
p c
p typeof(c)

d = {x: 10}
e = a.merge(d)
p e
p typeof(e)

@emancu
Copy link
Author

@emancu emancu commented May 25, 2016

@asterite do I need to update this PR with your suggested code or you are going to add it manually ?

@asterite
Copy link
Member

@asterite asterite commented May 26, 2016

@emancu We still need to define what semantics we want for the method, if we decide to include it. What happens with duplicated keys? If we look at Hash, it keeps the last key, so for me NamedTuple should do the same. If you want to combine tuples maybe we can have + that "adds" two named tuples. But I'm not sure, I'd need to see some use cases.

@miketheman
Copy link
Contributor

@miketheman miketheman commented Oct 2, 2016

#triage
When we last left off, this issue is waiting for @emancu to provide some more use cases to further the discussion of how the implementation of the NamedTuple +/merge method might work.

@emancu
Copy link
Author

@emancu emancu commented Sep 26, 2017

I guess we can close this PR now that #4688 was merged.

@asterite
Copy link
Member

@asterite asterite commented Sep 26, 2017

@emancu Indeed. Gracias! :-)

@asterite asterite closed this Sep 26, 2017
@straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Sep 30, 2017

I don't think this should be closed just because #4688 was merged, see #4727 (issue comment) and #4688 (issue comment).

@rdp
Copy link
Contributor

@rdp rdp commented Oct 21, 2017

Looks like #4727 and #4688 have been closed now FWIW. Thanks for doing this guys, I was about to request the same thing. See also https://groups.google.com/d/msg/crystal-lang/YnoO8GMovKQ/2ZWF3_IfDAAJ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants