Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Deprecate Sinatra::Reloader #40

Closed
TrevorBramble opened this Issue Mar 16, 2012 · 11 comments

Comments

Projects
None yet
5 participants
Contributor

TrevorBramble commented Mar 16, 2012

See: http://travis-ci.org/#!/TrevorBramble/sinatra-contrib/jobs/852727

This appears to actually be a spec failure due to how we're writing and loading temporary app files for these specs; a simple example app with sinatra/reloader required don't fall over.

Failure output extract:

51  1) Sinatra::Reloader automatically registers the reloader in the subclasses
52     Failure/Error: get('/foo').body.should == 'bar'
53       expected: "bar"
54            got: "foo" (using ==)
55     # ./spec/reloader_spec.rb:438:in `__script__'
56     # kernel/common/eval19.rb:45:in `instance_eval'
57     # kernel/bootstrap/array19.rb:18:in `map'
58     # kernel/bootstrap/array19.rb:18:in `map'
Owner

rkh commented Mar 16, 2012

Maybe we should move the reloader specs to something like the integration tests I added to Sinatra.

Contributor

TrevorBramble commented Mar 28, 2012

That's probably smart, but I think there's value in those specs where they are. I want to try replacing the file r/w activity with a mock of some kind.

Owner

zzak commented Aug 11, 2013

Perhaps another reason to discuss removal of sinatra/reloader

/cc sinatra/sinatra.github.com@7617305

@ashleygwilliams ashleygwilliams added the bug label May 18, 2015

Member

ashleygwilliams commented May 18, 2015

any further thought on this?

Owner

zzak commented May 18, 2015

We could probably deprecate and remove the reloader, but not sure

Contributor

TrevorBramble commented May 19, 2015

@ashleygwilliams @zzak I'm game for deprecating and removing it. If the Sinatra-specific reloader doesn't give us any value over a more generic tool, I'd rather see it gone. If someone wants to adopt it as a stand-alone mod, great.

@zzak zzak changed the title from Reloader fails under Rubinius 1.8 and 1.9 modes to Deprecate Sinatra::Reloader May 22, 2015

@zzak zzak added feature and removed bug labels May 22, 2015

@zzak zzak added this to the 1.5 milestone May 22, 2015

Owner

zzak commented Apr 19, 2016

Also, since the spec is no longer failing, I'm not sure there's any good reason to remove it.

I guess the reloader works for most people? There haven't been any bugs filed against it in a while.

In that case, why remove a feature if it's not causing any harm.

Contributor

stjhimy commented Apr 20, 2016

Last time I checked #179 still an issue.
I will run some tests tonight and get back to you.
Is there any valid alternative to sinatra/reloader? Shotgun only? https://github.com/rtomayko/shotgun

Owner

zzak commented Apr 21, 2016

After looking into that ticket, I'm not sure we could consider it a bug since it's out of the scope of the reloader. Although I'm not sure where it's explicitly written, based on the implementation, it's just calling require.

I'm not sure that's worth removing the feature all together.

Contributor

stjhimy commented Apr 23, 2016

Indeed, I did ran a few tests on the sinatra/reloader and it seems to be working fine.
The yml issue goes as you said, it's not a rb file so it's not reloaded.

We can close #40 and #196, if we decide to remove the reloader in the future then we can just merge #196.

@zzak zzak closed this May 3, 2016

Owner

zzak commented May 3, 2016

We'll keep the reloader for now, it's not causing such a major burden and some people still find it useful.

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