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

add setindex for named tuples #33468

Merged
merged 7 commits into from
Oct 10, 2019
Merged

Conversation

matbesancon
Copy link
Contributor

Given that Base.setindex is now documented, I think it makes some sense to have it defined for NamedTuples as well.
The implementation may be naive here and uses merge(::NamedTuple, itr), given that the left argument re-writes member already present in the first argument

@matbesancon
Copy link
Contributor Author

One possible improvement is to re-use the code from merge directly:

function merge(a::NamedTuple{an}, b::NamedTuple{bn}) where {an, bn}
    if @generated
        names = merge_names(an, bn)
        types = merge_types(names, a, b)
        vals = Any[ :(getfield($(sym_in(n, bn) ? :b : :a), $(QuoteNode(n)))) for n in names ]
        :( NamedTuple{$names,$types}(($(vals...),)) )

@matbesancon
Copy link
Contributor Author

cc @mbauman

@mbauman mbauman added the needs tests Unit tests are required for this change label Oct 4, 2019
@mbauman mbauman requested a review from JeffBezanson October 4, 2019 14:04
@matbesancon
Copy link
Contributor Author

I don't see how this creates a "test ambiguous" :/

Co-Authored-By: Matt Bauman <[email protected]>
@matbesancon matbesancon closed this Oct 6, 2019
@matbesancon matbesancon reopened this Oct 6, 2019
@matbesancon
Copy link
Contributor Author

re-opening for some failing checks

@matbesancon
Copy link
Contributor Author

I added a bunch of tests in addition to the docstring

@matbesancon
Copy link
Contributor Author

@mbauman I believe this is getting good. The failure is a connection problem

@mbauman
Copy link
Member

mbauman commented Oct 7, 2019

I have two questions:

  • Do we need two different setindex docstrings? Or can we define this in terms of a generic behavior that both ::Tuple and ::NamedTuple satisfy?
  • For ::NamedTuple, should this also accept an integer index as well?

@matbesancon
Copy link
Contributor Author

Do we need two different setindex docstrings? Or can we define this in terms of a generic behavior that both ::Tuple and ::NamedTuple satisfy?

In the case of NamedTuple, there isn't really a notion of out of bound for the symbol index. Otherwise we could merge the docstrings as you say, but do we need it?

For ::NamedTuple, should this also accept an integer index as well?

Is the syntax named_tuple[::Integer] really used much? It is not in the docstring of NamedTuple

@JeffBezanson
Copy link
Member

JeffBezanson commented Oct 9, 2019

I think this should support integer indices, but it has to be a separate method since it can't use merge directly.

matbesancon and others added 2 commits October 10, 2019 16:16
Co-Authored-By: Jeff Bezanson <[email protected]>
Co-Authored-By: Jeff Bezanson <[email protected]>
@JeffBezanson JeffBezanson removed the needs tests Unit tests are required for this change label Oct 10, 2019
@JeffBezanson JeffBezanson merged commit 49e2f41 into JuliaLang:master Oct 10, 2019
@tkf
Copy link
Member

tkf commented Oct 10, 2019

  • Do we need two different setindex docstrings? Or can we define this in terms of a generic behavior that both ::Tuple and ::NamedTuple satisfy?

I have an RFC on generic setindex specification: #33495. I think it covers Tuple and NamedTuple as well.

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

Successfully merging this pull request may close these issues.

4 participants