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 getindex methods for NamedTuple #38878

Merged
merged 6 commits into from
Jan 13, 2021
Merged

Conversation

kescobo
Copy link
Contributor

@kescobo kescobo commented Dec 14, 2020

See #38834

This adds getindex() methods for NamedTuples to return multiple values as a new NamedTuple, eg:

julia> nt = (a=1, b=2, c=3)
(a = 1, b = 2, c = 3)

julia> nt[(:a, :b)]
(a = 1, b = 2)

julia> nt[[:a, :b]]
(a = 1, b = 2)

julia> nt[[:a,]]
(a = 1,)

To be analogous with getindex for Tuples and Vectors. When Jeff commented on the issue, he only gave explicit approval to the idea of using a tuple of Symbols to index, not the AbstractVector, but it's easy enough to remove the latter if unwanted. If the AbstractVector method is approved, in the tests, I wasn't sure if testing all of the methods in 1 test would be a good idea or not (see comments) - I assumed keeping them separate would be better.

I also added a bit to the docstring, but I wasn't sure if I should also add something to base/docs/basedocs.jl somewhere around here

Open questions / todos

  • OK to keep getindex(t::NamedTuple, idxs::AbstractVector{Symbol}) ?
  • If yes, should tests for different methods be kept separate?
    • either way, remove comments in file
  • Should I add something to basedocs.jl?
  • Should I add a compat notice to docstring like the one here? If yes, I assume it would be for 1.7 at this point?

If there's anything else I've missed, please let me know!

@kescobo
Copy link
Contributor Author

kescobo commented Dec 14, 2020

More stupid typos 🤦 - got it fixed locally but will wait to push so as not to trigger extra CI time

@kescobo
Copy link
Contributor Author

kescobo commented Dec 22, 2020

Bump! Was wondering if maybe failing CI was keeping people from reviewing so I pushed the typo fix.

@@ -116,6 +119,8 @@ firstindex(t::NamedTuple) = 1
lastindex(t::NamedTuple) = nfields(t)
getindex(t::NamedTuple, i::Int) = getfield(t, i)
getindex(t::NamedTuple, i::Symbol) = getfield(t, i)
@inline getindex(t::NamedTuple, idxs::Tuple{Vararg{Symbol}}) = NamedTuple{idxs}(t)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if this has been discussed elsewhere, but why the @inline? Seems to not match the other methods around and should be simple enough inline by itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's how Jeff wrote it in the attached issue (#38834) - TBH I still don't totally understand what @inline does 🤷 . Happy to leave or remove

@kescobo
Copy link
Contributor Author

kescobo commented Jan 8, 2021

Happy New Year! Bump :-P

test/namedtuple.jl Outdated Show resolved Hide resolved
@kescobo
Copy link
Contributor Author

kescobo commented Jan 13, 2021

Alright, removed comments from tests, and also rebased onto current master branch. Open questions:

  1. should I addto basedocs,jl? I think not - looking again, other methods there are for constructors
  2. Should I add compat notice? I think it would be good, but want to double check that it should be for 1.7

@quinnj
Copy link
Member

quinnj commented Jan 13, 2021

Yeah, add a compat notice

@simeonschaub
Copy link
Member

Probably also worth adding an entry in NEWS.md

@simeonschaub simeonschaub added collections Data structures holding multiple items, e.g. sets needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change labels Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants