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: add Base.Iterators module #18839

Merged
merged 3 commits into from
Oct 18, 2016
Merged

RFC: add Base.Iterators module #18839

merged 3 commits into from
Oct 18, 2016

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Oct 7, 2016

Wraps the definitions that were in base/iterator.jl in a module, as has been discussed.

  • I found that zip and enumerate are so widely used that we should probably continue to export them from Base.
  • Deprecating the other previously-exported iterators is tricky, since if Base exports them as part of a deprecation, then using IterTools will conflict. We should add a rule that deprecated bindings can be shadowed instead of giving the "both X and Y export Z; uses of it must be qualified" error.
  • I was able to remove iterator.jl from coreimg.jl entirely, which is nice.
  • I separated Base.filter and IterTools.filter into two functions (since the latter is lazy and the former is eager). This is up for debate.

@ararslan
Copy link
Member

ararslan commented Oct 7, 2016

Perhaps the lazy filter from the submodule could be called lazyfilter? That avoids any potential name confusion.

For what it's worth (which I doubt is much), I'm personally not a huge fan of the name IterTools--sounds too "Pythonic." I think something like Iterators might be nicer. Anyway, just a thought.

@iamed2
Copy link
Contributor

iamed2 commented Oct 8, 2016

Iterators was rejected because Iterators.jl is a package.

@cstjean
Copy link
Contributor

cstjean commented Oct 8, 2016

I separated Base.filter and IterTools.filter into two functions (since the latter is lazy and the former is eager). This is up for debate.

Base.filter over iterators is lazy. #13712

@JeffBezanson
Copy link
Member Author

Yes, I separated the functions so that that's no longer the case. I guess I was a bit unclear; the parenthetical applies to the new functions.

@JeffBezanson JeffBezanson force-pushed the jb/itertools branch 6 times, most recently from 510de68 to 211d14d Compare October 11, 2016 05:05
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

lgtm if green

@JeffBezanson JeffBezanson changed the title add IterTools module RFC: add IterTools module Oct 11, 2016
@tkelman tkelman added the needs news A NEWS entry is required for this change label Oct 12, 2016
@tkelman
Copy link
Contributor

tkelman commented Oct 12, 2016

This is probably worth mentioning in NEWS.md

@kshyatt
Copy link
Contributor

kshyatt commented Oct 12, 2016

Needs a rebase and possibly that NEWS addition?

@JeffBezanson
Copy link
Member Author

Yep, and it occurs to me I should probably add a doc/stdlib file+section for the module.

@JeffBezanson JeffBezanson force-pushed the jb/itertools branch 2 times, most recently from cc7bfdb to 36f1f82 Compare October 12, 2016 22:20
@JeffBezanson JeffBezanson removed the needs news A NEWS entry is required for this change label Oct 12, 2016
@JeffBezanson JeffBezanson changed the title RFC: add IterTools module RFC: add Iterators module Oct 13, 2016
@JeffBezanson JeffBezanson changed the title RFC: add Iterators module RFC: add Base.Iterators module Oct 13, 2016
@JeffBezanson
Copy link
Member Author

We discussed this on the triage call. Consensus was that IterTools is an ugly name, and we don't want to get stuck with it. So now we have Base.Iterators as well as Iterators (the package). We felt that using the same name actually has better usability.

I tried this out, and the name conflict doesn't seem to be inconvenient. You can even do both using Iterators and using Base.Iterators and it works (aside from name conflicts, which in this case are easy to resolve by removing things from the Iterators package that are now in Base).

This will be more future-proof; we can potentially break up the Iterators package by (1) moving some things to Base.Iterators, (2) move more exotic iterators to their own packages. Then we'd be left with just Base.Iterators instead of just Base.IterTools.

@tkelman
Copy link
Contributor

tkelman commented Oct 13, 2016

wouldn't it be better to move everything from Base.Iterators to Iterators.jl and make it a default package?

@iamed2
Copy link
Contributor

iamed2 commented Oct 13, 2016

I feel like the best solution is to either move everything from Iterators.jl into Base (a la Rust) or reexport Base.Iterators from Iterators.jl so that users don't ever have to run into name conflicts.

@JeffBezanson
Copy link
Member Author

reexport Base.Iterators from Iterators.jl

That's a good idea.

One tricky thing about Base.Iterators is that it has some functions that are very widely used in Base, plus some things that are needed for lowering generators and comprehensions, so it can't be fully separated in any meaningful way (unless we split it into ImportantIterators and OtherIterators).

@tkelman
Copy link
Contributor

tkelman commented Oct 14, 2016

Seems that anything that isn't absolutely a dependency of all other code written in the language should eventually be a package, so important vs other is a fair dividing line between what should be here and what shouldn't.

@StefanKarpinski
Copy link
Member

You can pull that thread until there's no sweater left.

@tkelman
Copy link
Contributor

tkelman commented Oct 14, 2016

are we serious about #5155 or not?

@StefanKarpinski
Copy link
Member

Yes, but my point is that there's no hard line between what's necessary for Base and what isn't – and it can change over time. We should try to make the distinction a little less transient.

@JeffBezanson
Copy link
Member Author

Yeah I'm serious about it --- in that issue I list 26 modules to move out, without even mentioning this one.

@tkelman
Copy link
Contributor

tkelman commented Oct 14, 2016

Yes and that's a good list, but I don't see why the parts of Iterators that aren't needed for bootstrapping or lowering shouldn't be on it too.

@JeffBezanson
Copy link
Member Author

Maybe so; it's just a pretty small amount of code that's broadly applicable. Several of the iterators are <30 lines of code each, so moving them is a low priority. Some belong by symmetry; for example I wouldn't want take and drop in separate packages just because only one happened to be used in Base.

@@ -23,6 +23,7 @@ export
Docs,
Markdown,
Threads,
Iterators,
Copy link
Contributor

Choose a reason for hiding this comment

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

this probably shouldn't be exported, won't it conflict with the package name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I tried both ways, and exporting it didn't change the situation much. The way using works, if you do using Iterators first it loads the package with no warnings. All the other public submodules of Base are exported, so I decided to be consistent. However I'm willing to remove all or most of these exports as part of #5155; some will become top-level modules instead of submodules, and for the remainder we can change the policy on exporting them.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you do using Iterators without the package installed will it behave differently than with?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. using can't see Base.Iterators unless you do e.g. import Base.Iterators.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. But if exported from Base, does Iterators refer to something different before you do using Iterators vs after?

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case yes, and you get the usual WARNING: imported binding for Iterators overwritten in module Main.

Copy link
Member

Choose a reason for hiding this comment

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

it looks like this is now confusing precompile because it finds the Main.Iterators binding (probably using the wrong logic) and ends up having that shadow the real Iterators package. This shows up as breakage of the Compose package:

julia> using Compose
WARNING: The call to compilecache failed to create a usable precompiled cache file for module Compose. Got:
WARNING: Module Iterators uuid did not match cache file.

julia> using Iterators
WARNING: imported binding for Iterators overwritten in module Main

julia> using Compose

julia> 

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, precompile should use the equivalent of eval_import_path.

@lstagner
Copy link
Contributor

Perhaps #17935 could also be included with this

This removes a spurious warning if Base.X is deprecated to Mod.X,
and one does `using Mod`. `X` is available from both `Base` and
`Mod`, but we asked for the non-deprecated one so there should
be no warning.
@cstjean
Copy link
Contributor

cstjean commented Mar 3, 2017

On 0.6

julia> filter(x->true, (1,2))
WARNING: filter(flt, itr) is deprecated, use Iterators.filter(flt, itr) instead.
Base.Iterators.Filter{##3#4,Tuple{Int64,Int64}}(#3, (1, 2))

Shouldn't that be handled by Base.filter instead of Base.Iterators.Filter?

@yuyichao
Copy link
Contributor

yuyichao commented Mar 3, 2017

What should be handled by the deprecated Base.filter?

@cstjean
Copy link
Contributor

cstjean commented Mar 3, 2017

Sorry, I misunderstood the deprecation strategy. Never mind, it makes sense.

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.

10 participants