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 merge and friends with arbitrary number of args #130

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

pablosanjose
Copy link
Contributor

@pablosanjose pablosanjose commented Jan 21, 2024

Closes #129

adds methods for merge, merge!, mergewith and mergewith! with arbitrary number of AbstractDictionary args.

Copy link
Owner

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

Excellent - this is a great idea. Thanks for the contribution.

I think we should address Julia 1.0 compatibility (either drop it, or maintain it) and remove the Callable constraint, but other than that it looks good to me.

test/Dictionary.jl Outdated Show resolved Hide resolved
src/AbstractDictionary.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7b5b7b3) 79.35% compared to head (81a3062) 80.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   79.35%   80.10%   +0.74%     
==========================================
  Files          21       20       -1     
  Lines        2349     2357       +8     
==========================================
+ Hits         1864     1888      +24     
+ Misses        485      469      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pablosanjose
Copy link
Contributor Author

pablosanjose commented Jan 22, 2024

Thanks for the quick reply and review!
Regarding upping the compatibility requirement to 1.6, it's your call. I would personally do it, since 1.6 has been out very long now. Please let me know.
EDIT: Thinking about that again, I believe it's better practice to do the compat change in a separate PR.

@andyferris
Copy link
Owner

Re Julia 1.6, the question I have is this a breaking change? Do we need a major version bump? I suppose the package manager will deal with it for us.

@pablosanjose
Copy link
Contributor Author

pablosanjose commented Jan 22, 2024

Nope, it's not a breaking change (with the last commit). It can be argued that it is a bugfix (so bugfix release), since it just makes merge and co on AbstractDictionaries compliant with the API defined by Julia for AbstractDict.

@andyferris
Copy link
Owner

Sorry, my comment was about changing the minimum compatible Julia version to 1.6. In any case, such a decision can be dealt with seperately. (I agree this is more of a bugfix).

Thanks again!

@andyferris andyferris merged commit ae064a2 into andyferris:master Jan 22, 2024
23 checks passed
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.

No merge and friends with more than two AbstractDictionarys
2 participants