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 union(::OneTo, ::OneTo) #704

Merged
merged 4 commits into from
Jun 8, 2020
Merged

Conversation

goretkin
Copy link
Contributor

@goretkin goretkin commented Jun 1, 2020

I didn't bump the version number because I see there's another recent merge request.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

Just some small details, generally this looks good.

README.md Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
src/Compat.jl Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
goretkin and others added 2 commits June 3, 2020 20:14
Co-authored-by: Martin Holters <[email protected]>
@goretkin
Copy link
Contributor Author

goretkin commented Jun 3, 2020

I think this is good to go now! Thanks for the careful review, @martinholters

@martinholters
Copy link
Member

Hm, I'm having second thoughts as this changes the return type of union(::Base.OneTo, ::Base.OneTo) when Compat is loaded (on older Julia). It might be safer to instead define Compat.union to follow what Julia does in newer versions and leave Base.union alone.

In this case, I guess it's unlikely to cause any trouble (and JuliaLang/julia#35577 doesn't seem to have) so I'm in favor of merging as is, just wanted to point out this potential for problems and give others a bit more time to chime in.

@martinholters
Copy link
Member

Well, no objections voiced, so here we go...

@martinholters martinholters merged commit 234f390 into JuliaLang:master Jun 8, 2020
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.

2 participants