Skip to content

Conversation

@britto
Copy link
Contributor

@britto britto commented Nov 22, 2016

By warning about its blocking behavior and suggesting the use of Stream.iterate/2 for generating number sequences.

Copy link
Member

Choose a reason for hiding this comment

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

It is actually much more cheaper to use Stream.interval(0) than Stream.iterate(0, & &1 + 1):

iex> interval = Stream.interval(0)
iex> :timer.tc(Enum, :take, [interval, 100]) 
{36, ...}
iex> iterate = Stream.iterate(0, &(&1 + 1))
iex> :timer.tc(Enum, :take, [iterate, 100]) 
{184, ...}

Copy link
Member

@josevalim josevalim Nov 22, 2016

Choose a reason for hiding this comment

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

That's because you are using IEx so the anonymous function is being evaluated. Never benchmark in the shell. Doing the one below in a file gives me a slightly faster iterate:

interval = Stream.interval(0)
IO.inspect :timer.tc(Enum, :take, [interval, 1000])

iterate = Stream.iterate(0, &(&1 + 1))
IO.inspect :timer.tc(Enum, :take, [interval, 1000])

Copy link
Member

Choose a reason for hiding this comment

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

Riiight, though with compiled code there is no clear winner:

{103, ...}
{106, ...}
======
{103, ...}
{98, ...}
======
{103, ...}
{124, ...}
======
{103, ...}
{98, ...}

I believe there is no meaningful difference.

Copy link
Member

Choose a reason for hiding this comment

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

Right. So let's go with the one that is better semantically. :)

Copy link
Member

Choose a reason for hiding this comment

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

Can we please remove the bold and say "instead use ..." or "use ... instead". I believe the "use instead" is not idiomatic. :)

Copy link
Member

Choose a reason for hiding this comment

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

We use use X instead. in every other place.

@josevalim
Copy link
Member

I am 👍 because it gives a nudge in the proper direction. I actually have no use cases for Stream.interval and Stream.timer besides possibly testing. They were added in a time we thought Stream would have async functionality but it never became true. It is in my plans to evaluate deprecating them so pushing users away from it is a good idea.

@britto britto force-pushed the improve-stream-interval-doc branch from f5ac129 to 480179f Compare November 22, 2016 18:48
@gusaiani
Copy link
Contributor

gusaiani commented Nov 22, 2016

@britto @josevalim Mind there's a necesary in the comment, instead of necessary.

@lexmag
Copy link
Member

lexmag commented Nov 22, 2016

With this addition we have conflicting usage example: we generate sequence of numbers. :bowtie:

@britto
Copy link
Contributor Author

britto commented Nov 22, 2016

@gusaiani thanks! fixed.

@britto britto force-pushed the improve-stream-interval-doc branch from 480179f to 69267c7 Compare November 22, 2016 18:54
Copy link
Member

Choose a reason for hiding this comment

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

Probably we should have parens here, I think better to be closer to what Macro.to_string/1 produce.

Copy link
Member

Choose a reason for hiding this comment

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

I find it much more readable without the parenthesis. Maybe we should make Macro.to_string/1 smart enough to handle those cases. It has a tendency to add too many parens. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Just applied your suggestions 👍.

Copy link
Member

@lexmag lexmag Nov 22, 2016

Choose a reason for hiding this comment

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

Oh noes. "parens" is the answer to life, the universe, and everything.

Copy link
Member

Choose a reason for hiding this comment

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

Amount of people who confused by dangling & requires parens. :)

Copy link
Member

Choose a reason for hiding this comment

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

We had this discussion in the past, so no need to revisit it here. :) It is part of the language, it is here to stay, folks may as well get acquainted with it.

Copy link
Member

Choose a reason for hiding this comment

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

But I am not folks! I don't want to get acquainted! 😄

@britto
Copy link
Contributor Author

britto commented Nov 22, 2016

I aggree that the current documentation is pretty self-explanatory. My point here is to warn beginners who might overlook the sleep implication of this function call. Since I've seen some misuse of this function in the wild, I thought it would be a good idea to reinforce the warning.

@britto britto force-pushed the improve-stream-interval-doc branch from 69267c7 to c54048f Compare November 22, 2016 19:00
By warning about its blocking behavior and suggesting the use
of `Stream.iterate/2` for generating number sequences.
@britto britto force-pushed the improve-stream-interval-doc branch from c54048f to b00f06b Compare November 22, 2016 19:02
@josevalim josevalim merged commit 0809ef5 into elixir-lang:master Nov 22, 2016
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@britto britto deleted the improve-stream-interval-doc branch November 22, 2016 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants