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

New Router implementation #29

Closed
wants to merge 3 commits into from
Closed

Conversation

david415
Copy link

Dear @glyph, here's a new router implementation I came up with; I'm curious to hear what you think of it.

Review on Reviewable

@glyph
Copy link
Member

glyph commented Oct 25, 2015

What was deficient in the existing implementation?

@glyph
Copy link
Member

glyph commented Oct 25, 2015

This definitely isn't going to land as-is, since it:

  • deletes almost all the existing documentation which explains how to use Router
  • removes the stuff that makes fanchat.py work but doesn't replace it
  • fails the tests
  • removes type safety on routed vs. non-routed item types, making easier to make an error
  • implements its own Fount and Drain (which the existing router had avoided for specifically this reason) while not handling numerous edge cases around what happens when switching upstream/downstream while paused.

I also have to guess why you wanted a new routing implementation, but my first guess is that you wanted to add the getItemDestination argument, allowing you to make the routing decision inside the router rather than having the sender know where they're delivering things to.

@david415
Copy link
Author

I wrote this because I couldn't get your existing Router to work... I tried and documented my failed attempts here in the form of unit tests: #28

@glyph
Copy link
Member

glyph commented Oct 26, 2015

Whoops! Thanks for the bug report.

@glyph
Copy link
Member

glyph commented Oct 26, 2015

@david415 I'm going to try to get the Router implementation fixed and tested. Sorry about that.

@glyph
Copy link
Member

glyph commented Nov 25, 2015

I have fixed the most egregious problems with routing now, and I've filed #38 and its related tickets to deal with the more elaborate parts of flow-control where it initially seems like something like QueueFount might be useful (but in fact the solution is something else). And I've opened #39 to clean up the document that gave you this mistaken impression.

Probably these need clarification but discussion should move to the more specific issues now. I'm going to close this since I think the purpose it was meant to serve has been adequately dealt with elsewhere for now.

Thanks again for taking Tubes for a spin!

@glyph glyph closed this Nov 25, 2015
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