JuliaCollections / IterTools.jl

Common functional iterator patterns
Other
154 stars 29 forks source link

Update capitalization for iterator traits #22

Closed iamed2 closed 6 years ago

iamed2 commented 6 years ago

For 0.7 compat

ararslan commented 6 years ago

I think these are available in Compat.

iamed2 commented 6 years ago

Compat's version doesn't allow dispatch on IteratorSize on 0.6

iamed2 commented 6 years ago

Ugh apparently this doesn't work

iamed2 commented 6 years ago

Do I have to import them and make the const aliases?

ararslan commented 6 years ago

What I'd recommend:

@static if VERSION < v"0.7.0-DEV.3309"
    const _iteratorsize = Base.IteratorSize
    const _iteratoreltype = Base.IteratorEltype
else
    const _iteratorsize = Base.iteratorsize
    const _iteratoreltype = Base.iteratoreltype
end

That way you should be able to safely overload _iteratorsize and _iteratoreltype, and the types themselves can be referenced as Base.IteratorSize and Base.IteratorEltype across versions.

iamed2 commented 6 years ago

My latest approach works once I fixed the typo. What do you think @ararslan ?

As a side note, it's pretty annoying to work with two versions of Julia on one package right now.

codecov-io commented 6 years ago

Codecov Report

Merging #22 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #22   +/-   ##
=======================================
  Coverage   93.36%   93.36%           
=======================================
  Files           2        2           
  Lines         392      392           
=======================================
  Hits          366      366           
  Misses         26       26
Impacted Files Coverage Δ
src/IterTools.jl 93.28% <ø> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 747400b...2bdf77f. Read the comment docs.

ararslan commented 6 years ago

What do you think @ararslan?

If it works for you and it works for CI, it works for me. 🙂

martinholters commented 6 years ago

Just came across this after seeing an annoying spew of deprecation warnings... So I have no idea about the inner workings of this package, but couldn't you use Compat.IteratorSize for the function and Base.IteratorSize for the type? Of course, your current status is perfectly working, but re-introducing iteratorsize seems so backwards...

EDIT: Just noticed that Compat isn't required at the moment, so might not be worth pulling it in just for this.

iamed2 commented 6 years ago

I think the existing approach is pretty simple; it just replicates the old environment so no functions or types are changed.

Will merge soon.