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.

Sign up for GitHub

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

Jump to bottom

delete for NamedTuples #27725

Closed
wants to merge 36 commits into from
Closed

delete for NamedTuples #27725

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e747501
add `dropnames`
JeffreySarnoff Jun 22, 2018
fdd3a98
add dropnames tests
JeffreySarnoff Jun 22, 2018
a239411
change `dropnames` to `delete`
JeffreySarnoff Jun 22, 2018
36af455
change `dropnames` to `delete`
JeffreySarnoff Jun 22, 2018
e33cea2
export `delete` (namedtuples)
JeffreySarnoff Jun 22, 2018
fc46f7a
shorten docstring for `delete`
JeffreySarnoff Jun 22, 2018
0338959
add `delete(::NamedTuple, ::Symbol)`
JeffreySarnoff Jun 22, 2018
0bc6e80
add tests for `delete(::NamedTuple, ::Symbol)`
JeffreySarnoff Jun 22, 2018
354d543
another test for `delete(::NamedTuple, ::Symbol)`
JeffreySarnoff Jun 22, 2018
cf160bb
reword delete docstring for clarity
JeffreySarnoff Jun 23, 2018
ec186c5
rework delete docstring for clarity
JeffreySarnoff Jun 23, 2018
524bc8b
test typeof(delete(..)) is fully specified
JeffreySarnoff Jun 23, 2018
65aac32
add, comment out @inferred for delete
JeffreySarnoff Jun 23, 2018
db79715
uncomment @inferrable tests
JeffreySarnoff Jun 23, 2018
14dbfd2
use recommended @inferred test
JeffreySarnoff Jun 23, 2018
292d88e
remove test `@inferred delete`
JeffreySarnoff Jun 24, 2018
619fe4c
Base.delete --> delete in tests
JeffreySarnoff Jun 25, 2018
3606bec
comment out delete tests (see what circleci does)
JeffreySarnoff Jun 29, 2018
d43cf81
temp comment out delete() to see what circleci does
JeffreySarnoff Jun 29, 2018
99a3d20
rename delete(nt, tuple) to deleteindicies, change docstr
JeffreySarnoff Jun 29, 2018
be15cc1
export deleteindicies
JeffreySarnoff Jun 29, 2018
a4abb90
test deleteindicies
JeffreySarnoff Jun 29, 2018
2f37e71
Update namedtuple.jl
JeffreySarnoff Jun 29, 2018
308deb6
Update namedtuple.jl
JeffreySarnoff Jun 29, 2018
e1ae2de
delete(nt, :sym), delete(nt, :sym1, :sym2)
JeffreySarnoff Jun 30, 2018
86a4b12
replace deleteindicies with delete
JeffreySarnoff Jun 30, 2018
5226c03
remove space
JeffreySarnoff Jun 30, 2018
ea3aeae
remove deleteindicies
JeffreySarnoff Jun 30, 2018
4704588
test delete with no fieldnames
JeffreySarnoff Jun 30, 2018
2202264
corrected signature, docstring
JeffreySarnoff Jun 30, 2018
b10fced
fix typo
JeffreySarnoff Jun 30, 2018
6f6f25b
fixup delete test
JeffreySarnoff Jun 30, 2018
993cc2b
Update namedtuple.jl
JeffreySarnoff Jul 4, 2018
54198b3
Fix signature in docstring
nalimilan Jul 5, 2018
14d0b46
delete limited to one symbol
JeffreySarnoff Jul 5, 2018
5559d8e
delete limited to one symbol
JeffreySarnoff Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions base/exports.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,7 @@ export
firstindex,
collect,
count,
delete,
delete!,
deleteat!,
eltype,
Expand Down
10 changes: 10 additions & 0 deletions base/namedtuple.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,16 @@ get(f::Callable, nt::NamedTuple, key::Union{Integer, Symbol}) = haskey(nt, key)
(names...,)
end

"""
delete(nt::NamedTuple{an}, omitnames::Tuple{Vararg{Symbol}}) where {an}
Copy link

Choose a reason for hiding this comment

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

@JeffreySarnoff do you think it is really necessary to give the parametric type in the named tuple? I don't think the {an} where {an} adds something and makes the docstring longer and less readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the form used in another NamedTuple function. The an captures the names directly, rather than requiring a separate function call.

Copy link

Choose a reason for hiding this comment

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

I am referring to the documentation string. Should the user care about this implementation detail? Isn't it anyway clear that a named tuple is parametricaly defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

really, though for the docstring could be given without that and still be valid -- just not matching
I have no idea if that is frowned upon.

"""
    delete(nt::NamedTuple, omitnames::Tuple{Varag{Symbol}})
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will shorten the doc string -- and if there are voices to put it back -- I will do that.


Construct a copy of a named tuple `nt`, omitting the fields named in `omitnames`.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps copy doesn't have to be mentioned since tuples are immutable?

"""
function delete(a::NamedTuple{an}, omitnames::Tuple{Vararg{Symbol}}) where {an}
names = diff_names(an, omitnames)
NamedTuple{names}(a)
end

"""
structdiff(a::NamedTuple{an}, b::Union{NamedTuple{bn},Type{NamedTuple{bn}}}) where {an,bn}

Expand Down
6 changes: 6 additions & 0 deletions test/namedtuple.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,12 @@ end
abstr_nt_22194_3()
@test Base.return_types(abstr_nt_22194_3, ()) == Any[Any]

@test Base.delete((a=1, b=2), (:b,)) == (a=1,)
Copy link
Member

Choose a reason for hiding this comment

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

I think I read that the result here should be inferrable? If so, should these test also check that?

Copy link
Contributor Author

@JeffreySarnoff JeffreySarnoff Jun 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How do I test that? Guessed this for that.
@test typeof(delete((a=1.0, b="two"), :b)) == NamedTuple{(:a,), Tuple{Float64}}

Copy link
Member

Choose a reason for hiding this comment

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

@inferred should work.

Copy link
Member

Choose a reason for hiding this comment

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

Also Base. isn't needed, right?

@test Base.delete((a=1, b=2), (:a,)) == (b=2,)
@test Base.delete((a=1, b=2, z=20), (:b,)) == (a=1, z=20)
@test Base.delete((a=1, b=2, z=20), (:b, :q, :z)) == (a=1,)
@test Base.delete((a=1, b=2, z=20), (:b, :q, :z, :a)) == NamedTuple()

@test Base.structdiff((a=1, b=2), (b=3,)) == (a=1,)
@test Base.structdiff((a=1, b=2, z=20), (b=3,)) == (a=1, z=20)
@test Base.structdiff((a=1, b=2, z=20), (b=3, q=20, z=1)) == (a=1,)
Expand Down