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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override Rack::ShowExceptions#pretty to set custom template #1377

Merged
merged 1 commit into from Feb 6, 2018

Conversation

Projects
None yet
2 participants
@jkowens
Member

jkowens commented Jan 6, 2018

This overrides Rack::ShowExceptions::TEMPLATE without affecting other Rack apps that are mounted alongside Sinatra. Resolves #1376.

I would like to revisit this if rack/rack/pull/1223 gets merged 馃槣

Override #pretty method to set custom html template
This overrides Rack::ShowExceptions::TEMPLATE without affecting other
Rack apps that are mounted alongside Sinatra.
@namusyaka

This looks good to me.

In my opinion, if the rack pr will be accepted, it means that the way is standard for defining custom template in any sub frameworks, but our current issue should be considered separately from that.

I also thought about defining the Sinatra::ShowException class as a completely unique class apart from those of rack, like ActionDispatch::ShowExceptions
However our exception handling depends on env['rack.errors'], so at least at the moment we should inherit and extend Rack :: ShowExceptions.

@namusyaka namusyaka added this to the v2.0.1 milestone Feb 6, 2018

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Feb 6, 2018

Member

@jkowens I'm going to merge this PR, thanks for your patch.
Please let me know if there are any worries.

Member

namusyaka commented Feb 6, 2018

@jkowens I'm going to merge this PR, thanks for your patch.
Please let me know if there are any worries.

@namusyaka namusyaka merged commit 6094e4a into sinatra:master Feb 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jszwedko jszwedko referenced this pull request Jul 17, 2018

Merged

Update sinatra to 2.0.3 #77

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