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

Inconsistent overriding of previously-defined helpers #1213

Closed
mwpastore opened this issue Dec 7, 2016 · 2 comments
Closed

Inconsistent overriding of previously-defined helpers #1213

mwpastore opened this issue Dec 7, 2016 · 2 comments
Labels
Milestone

Comments

@mwpastore
Copy link
Member

@mwpastore mwpastore commented Dec 7, 2016

There's a slight inconsistency with the order in which Sinatra applies helpers when those helpers may have been previously defined. This is probably best explained with an example:

module HelperOne
  def one; '1' end
end

class MockApp < Sinatra::Base
  helpers do
    def one; nil end
    def two; nil end
  end

  helpers ::HelperOne do
    def two; '2' end
  end

  get('/one') { one }
  get('/two') { two }
end

One might reasonably expect one of two outcomes from the above:

  1. /one returns "1" and /two returns "2" (i.e. the later-defined helpers override the previously-defined helpers)
  2. /one returns nil and /two returns nil (i.e. the previously-defined helpers are "sticky" and cannot be overridden)

But what actually happens is:

  1. /one returns nil and /two returns "2"

So previously-defined helpers can be overridden in the block, but not in an included helpers module. This is inconsistent and confusing. To fix it, let's prepend the module(s) instead of include-ing them.

mwpastore added a commit to mwpastore/sinatra that referenced this issue Dec 7, 2016
mwpastore added a commit to mwpastore/sinatra that referenced this issue Dec 7, 2016
@zzak
Copy link
Member

@zzak zzak commented Dec 14, 2016

I'm not sure extent the affect of changing to prepend could have on existing applications, so my gut is to hold off on this.

@zzak zzak added the feedback label Dec 14, 2016
@zzak zzak added this to the Beyond milestone Dec 14, 2016
@namusyaka namusyaka modified the milestones: Beyond, v2.1.0 Mar 6, 2018
@namusyaka
Copy link
Contributor

@namusyaka namusyaka commented Mar 6, 2018

Looks reasonable.

jkowens added a commit to mwpastore/sinatra that referenced this issue Mar 18, 2020
@jkowens jkowens closed this in 2db247d Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.