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

Listen 3x proposal #318

Merged
merged 47 commits into from
Jun 26, 2015
Merged

Listen 3x proposal #318

merged 47 commits into from
Jun 26, 2015

Conversation

e2
Copy link
Contributor

@e2 e2 commented Jun 12, 2015

See #319 for details

Commit messages are self-explanatory, including:

  • removing TCP functionality
  • removing Celluloid

@e2 e2 added this to the 3.0.0 milestone Jun 12, 2015
@e2 e2 force-pushed the listen_3x_proposal branch from 262b3a1 to 1e054d7 Compare June 12, 2015 21:33
require 'listen/listener'

require 'listen/internals/thread_pool'

# Always set up logging by default first time file is requried
Copy link
Member

Choose a reason for hiding this comment

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

Typo spotted requried => required.

@mechanicalduck
Copy link

Remove TCP functionality - does this mean that Listen cannot be used over network anymore?

@rymai
Copy link
Member

rymai commented Jun 15, 2015

No, it's moved into two separate gems (see #319). ;)

transition :backend_started if state == :initializing
transition :frontend_ready if state == :backend_started
transition :processing_events if state == :frontend_ready
transition :processing_events if state == :paused
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a case statement here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because those steps are in a sequence - e.g. line 91 can set :frontend_ready and line 92 tests for that. We can't "jump" from :processing to :paused, because we have to go through the states in between (which is what's happening here). Perhaps the last line could be if [:paused, :processing_events].include?(state).

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok I get it, thanks for the explanation!

@rymai
Copy link
Member

rymai commented Jun 15, 2015

This PR looks definitely good. I'll try using it a bit and give you some feedback on how it works / performs for me!

Thanks for the hard work @e2!

@e2
Copy link
Contributor Author

e2 commented Jun 15, 2015

Thanks for finding time to go through this! There's lots more that could be done.

@e2 e2 changed the title (NOT FOR MERGE YET) Listen 3x proposal Listen 3x proposal Jun 26, 2015
@e2 e2 force-pushed the listen_3x_proposal branch from 329f209 to bf98fc8 Compare June 26, 2015 19:01
@e2
Copy link
Contributor Author

e2 commented Jun 26, 2015

@rymai - if you have no objections, I'd like to release this as Listen 3.0. I want to get this version working with Spring ASAP, so people can finally start using Rails, recent Listen and Guard version without issues.

While I took lots of care to make sure there are zero problems, even if I broke something, this won't affect many people, since lots of tools are locked to Listen 2.x (e.g. Guard), and if there are any issues, I prefer to discover them as soon as possible.

So I don't see a reason for a pre-release, since for me this is master, and it's what I prefer to fix, debug and maintain - to me it's closer to "production ready" than Listen 2.x.

Let me know if you disagree.

e2 added a commit that referenced this pull request Jun 26, 2015
@e2 e2 merged commit 457c3cb into master Jun 26, 2015
@e2 e2 deleted the listen_3x_proposal branch June 26, 2015 20:05
@rymai
Copy link
Member

rymai commented Jun 27, 2015

No problem for me. I've been using this branch this week and it worked as a charm! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants