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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added recursive merge function for NamedTuple, addresses #29215 #29259

Merged
merged 7 commits into from Nov 1, 2018

Conversation

3 participants
@jpsamaroo
Copy link
Contributor

commented Sep 18, 2018

I only added a single test because I'm highly uncreative; please let me know if there are any more I should add.

P.S. This is my first attempt at contributing to Julia proper, so please let me know if I screwed anything up 馃槃

@jpsamaroo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

Screwed up the docstring slightly, another commit is on the way (including another simple test, because why not)...

"""
merge(a::NamedTuple, b::NamedTuple...)
Perform a recursive merge of two or more named tuples.

This comment has been minimized.

Copy link
@JeffBezanson

JeffBezanson Sep 19, 2018

Member

I don't really see this as recursive; maybe we could describe it as left-associative?

This comment has been minimized.

Copy link
@jpsamaroo

jpsamaroo Sep 19, 2018

Author Contributor

Oh, it's not actually recursive... Clearly I didn't actually test this properly 馃槃
I can change the docstring for this method to indicate that it's left-associate instead; should I also take a stab at making a proper recursive version for this PR (and if so, what should it be named, or should it just be a recursive::Bool kwarg to the regular merge)?

(a = 1, b = 3, c = (d = 2,))
```
"""
merge(a::NamedTuple, b::NamedTuple...) = merge(merge(a, b[1]), b[2:end]...)

This comment has been minimized.

Copy link
@JeffBezanson

JeffBezanson Sep 19, 2018

Member

This would be a bit more elegant:

merge(a::NamedTuple, b::NamedTuple, cs::NamedTuple...) = merge(merge(a, b), cs...)

This comment has been minimized.

Copy link
@jpsamaroo

jpsamaroo Sep 19, 2018

Author Contributor

Agreed, and now changed

jpsamaroo added some commits Sep 19, 2018

@JeffBezanson JeffBezanson added this to the 1.1 milestone Sep 19, 2018

"""
merge(a::NamedTuple, b::NamedTuple, cs::NamedTuple...)
Perform a left-associative merge of three or more named tuples.

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Sep 20, 2018

Member

Why not two?

This comment has been minimized.

Copy link
@jpsamaroo

jpsamaroo Sep 20, 2018

Author Contributor

Because regular merge(a::NamedTuple, b::NamedTuple) already handles the two-argument case? I wrote the docstring for just this single method, which accepts only three or mpre arguments. Should I have done it differently?

This comment has been minimized.

Copy link
@fredrikekre

fredrikekre Sep 20, 2018

Member

Maybe we can combine the two docstrings then?

This comment has been minimized.

Copy link
@JeffBezanson

This comment has been minimized.

Copy link
@jpsamaroo

jpsamaroo Oct 20, 2018

Author Contributor

What's the best way to merge the docstrings, just copy one of them into the other after 2 newlines and leave this new merge definition without its own docstring? If so, is it ok if I replace merge(a::NamedTuple, b::NamedTuple) definition in the docstring with merge(a::NamedTuple, bs::NamedTuple...), even though none of the individual methods have that exact signature?

@jpsamaroo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

Do we also want a 1-argument fallback like @peterahrens had in #29362 ? I think this would be useful when splatting into merge. I would add a line in the docstring indicating that 1-argument merge is also implemented as a fallback.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

Yes that would be fine. merge(a::NamedTuple, bs::NamedTuple...) matches one or more arguments, so that is already implied by the existing doc string.

@JeffBezanson JeffBezanson merged commit 63f1c16 into JuliaLang:master Nov 1, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

fredrikekre added a commit that referenced this pull request Nov 30, 2018

News and compat annotation for #29259
(merge(::NamedTuple...) with more than 2 arguments).

fredrikekre added a commit that referenced this pull request Dec 1, 2018

News and compat annotation for #29259
(merge(::NamedTuple...) with more than 2 arguments).

fredrikekre added a commit that referenced this pull request Dec 1, 2018

News and compat annotation for #29259
(merge(::NamedTuple...) with more than 2 arguments).

fredrikekre added a commit that referenced this pull request Dec 3, 2018

News and compat annotation for #29259
(merge(::NamedTuple...) with more than 2 arguments).

fredrikekre added a commit that referenced this pull request Dec 4, 2018

News and compat annotation for #29259
(merge(::NamedTuple...) with more than 2 arguments).

fredrikekre added a commit that referenced this pull request Dec 4, 2018

News and compat annotation for #29259
(merge(::NamedTuple...) with more than 2 arguments).

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Compat admonitions and NEWS for Julia 1.1 (#30230)
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.