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

Fix #compiled_router not working well. #1381

Merged
merged 2 commits into from Aug 18, 2013

Conversation

Projects
None yet
2 participants
@namusyaka
Member

namusyaka commented Aug 18, 2013

Condition was inappropriate, so fixed it.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Aug 18, 2013

Member

Is deferred_routes[1] or [0] or [2] ever empty? I fail to see a point in this condition at all.

Member

ujifgc commented Aug 18, 2013

Is deferred_routes[1] or [0] or [2] ever empty? I fail to see a point in this condition at all.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Aug 18, 2013

Member

deferred_routes default return value is [[], [], []] (the same is true if it is compiled once).
If the condition is deferred_routes.empty?, not run forever.

Member

namusyaka commented Aug 18, 2013

deferred_routes default return value is [[], [], []] (the same is true if it is compiled once).
If the condition is deferred_routes.empty?, not run forever.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Aug 18, 2013

Member

I mean, I can't see any test case that would require checking every priority in this array for emptiness.

How about this?

  def compiled_router
    if @deferred_routes
      deferred_routes.each { |routes| routes.each { |(route, dest)| route.to(dest) } }
      @deferred_routes = nil
      router.sort!
    end
    router
  end

In the previous code every #url we call, the #compiled_router is invoked and it checks the deferred routes again and again.

Member

ujifgc commented Aug 18, 2013

I mean, I can't see any test case that would require checking every priority in this array for emptiness.

How about this?

  def compiled_router
    if @deferred_routes
      deferred_routes.each { |routes| routes.each { |(route, dest)| route.to(dest) } }
      @deferred_routes = nil
      router.sort!
    end
    router
  end

In the previous code every #url we call, the #compiled_router is invoked and it checks the deferred routes again and again.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Aug 18, 2013

Member

Makes sense.
That code is good, and it is better than my code.

Should I commit your code in this pull request?

Member

namusyaka commented Aug 18, 2013

Makes sense.
That code is good, and it is better than my code.

Should I commit your code in this pull request?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Aug 18, 2013

Member

Yep.

Member

ujifgc commented Aug 18, 2013

Yep.

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Aug 18, 2013

Member

I'm done.
Thanks @ujifgc!

Member

namusyaka commented Aug 18, 2013

I'm done.
Thanks @ujifgc!

ujifgc added a commit that referenced this pull request Aug 18, 2013

Merge pull request #1381 from namusyaka/fix-compiled_router
Fix #compiled_router not working well.

@ujifgc ujifgc merged commit da174aa into padrino:master Aug 18, 2013

1 check passed

default The Travis CI build passed
Details

@namusyaka namusyaka deleted the namusyaka:fix-compiled_router branch Aug 18, 2013

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