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

RFC: Remove obsolete custom Iterators module #550

Merged
merged 1 commit into from
May 23, 2018
Merged

Conversation

martinholters
Copy link
Member

Our tests have started failing because

julia> @test collect(rest(1:10, 5)) == [5,6,7,8,9,10]
Test Failed at REPL[13]:1
  Expression: collect(rest(1:10, 5)) == [5, 6, 7, 8, 9, 10]
   Evaluated: [6, 7, 8, 9, 10] == [5, 6, 7, 8, 9, 10]

the state for iterating over a range has changed its meaning. (Arguably, it's a bad idea to depend on the meaning of iteration state like that.)

As on all supported Julia versions, we just load Base.Iterators, there is no point in defining our own module, and then I hope we can just remove all those tests instead of developing elaborate alternatives.

There is a twist: We've been using Base.Iterators (if present), meaning that e.g. Compat.zip(...) would have worked, but only since Julia 0.6, where Iterators.zip(...) without any Compat worked just as well. Anyone actually aiming at compatibility with Julia 0.5 had to use Compat.Iterators, so I guess switching from using to import here should not break anyones code. At some point, we should also deprecate that binding, but I'm not sure whether that point is now.

@ararslan
Copy link
Member

This is an odd failure:

Test Failed at /home/travis/.julia/v0.7/Compat/test/runtests.jl:676
  Expression: Dates.Day(30) == Dates.Month(1)
    Expected: MethodError
  No exception thrown
ERROR: LoadError: There was an error during testing
in expression starting at /home/travis/.julia/v0.7/Compat/test/runtests.jl:676

Oddly enough it seems not to be affecting macOS 0.7, only Linux and Windows...

@martinholters
Copy link
Member Author

Is say that's due to JuliaLang/julia#27165 and OSX binary isn't as new as the others.

@ararslan
Copy link
Member

Ah, good catch. I suppose we could wrap that test in a version check, or perhaps just remove it from the Compat test suite?

@ararslan ararslan mentioned this pull request May 22, 2018
@martinholters
Copy link
Member Author

These should probably give false to match current Base behavior:

Compat.jl/src/Compat.jl

Lines 449 to 450 in c5527e9

==(x::Dates.FixedPeriod, y::Dates.OtherPeriod) = throw(MethodError(==, (x, y)))
==(x::Dates.OtherPeriod, y::Dates.FixedPeriod) = throw(MethodError(==, (x, y)))

Making an error a non-error should be non-breaking, so I don't see any harm in changing that. Will open a PR shortly...

@martinholters
Copy link
Member Author

Ah no, looking again, the Dates comparison stuff is only relevant prior to 0.6, so I'll just remove it completely.

@ararslan ararslan closed this May 23, 2018
@ararslan ararslan reopened this May 23, 2018
@ararslan ararslan merged commit 5b40ef1 into master May 23, 2018
@ararslan ararslan deleted the mh/rm_iterators branch May 23, 2018 16:27
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