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

Update ShowExceptions to suit latest rack master #907

Merged
merged 1 commit into from Aug 11, 2014

Conversation

Projects
None yet
8 participants
@kgrz
Member

kgrz commented Jul 24, 2014

  • Post 893a2c50 in rack/rack, the #pretty method used while generating
    the HTML error markup, implemented in Rack::ShowExceptions, returns a
    String instead of an Array. This change uses Array() to
    convert the exception string in both plain text and HTML modes
    to an array.
  • Should fix #906
Update ShowExceptions to suit latest rack master
* Post 893a2c50 in rack/rack, the #pretty method used while generating
  the HTML error markup, implemented in Rack::ShowExceptions, returns a
  String instead of an Array. This change uses Array() to
  convert the exception string in both plain text and HTML modes
  to an array.
@kgrz

This comment has been minimized.

Member

kgrz commented Jul 24, 2014

Although this seems to fix that error, the discussion in that commit is suggesting me that it's better to implement the pretty method in Sinatra than using the one provided in Rack. Thoughts?

@kytrinyx

This comment has been minimized.

Member

kytrinyx commented Jul 25, 2014

This is tricky. Did rack master really make a breaking change? Are they going to make a major version change? (Sorry, I've not been following this issue, so I'm pretty ignorant about the situation. If nobody knows off the top of their head I'll go look for some answers myself).

@rkh

This comment has been minimized.

Member

rkh commented Jul 25, 2014

I think this might not be a public Rack API we're using here. Also, Rack will not have a major release any time soon. Though I don't think Rack officially follows semantic versioning (this is further complicated by the Rack version being related to the Rack specification version).

@vipulnsward

This comment has been minimized.

Contributor

vipulnsward commented Jul 25, 2014

Yes it is not public API and meant to change in future. We should instead do a custom implementation, instead of handling for different versions.

else
content_type = "text/html"
body = pretty(env, e)
exception_string = pretty(env, e)

This comment has been minimized.

@zzak

zzak Aug 11, 2014

Member

FYI pretty() is not meant to be public so this may bite us again, but I'm ok with merging it for now.

zzak added a commit that referenced this pull request Aug 11, 2014

Merge pull request #907 from kgrz/fix-error-with-rack-master
Update ShowExceptions to suit latest rack master

@zzak zzak merged commit a43ba2c into sinatra:master Aug 11, 2014

1 check passed

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

joshuap added a commit to honeybadger-io/honeybadger-ruby that referenced this pull request Dec 29, 2014

ghost pushed a commit to flapjack/flapjack that referenced this pull request Jan 12, 2015

@philostler

This comment has been minimized.

philostler commented Jan 19, 2015

Can anyone tell me when this will make it into a release? I get a lot of breaking specs from this

@rkh

This comment has been minimized.

Member

rkh commented Jan 20, 2015

Will cut a new release later this week or early next.

@philostler

This comment has been minimized.

philostler commented Jan 20, 2015

Thanks, appreciated! 👍

@ringe

This comment has been minimized.

ringe commented Feb 10, 2015

Is the new release far away?

rubys added a commit to rubys/wunderbar that referenced this pull request Feb 10, 2015

@aldanor

This comment has been minimized.

aldanor commented Feb 20, 2015

Will this be released soon?

@kytrinyx

This comment has been minimized.

Member

kytrinyx commented Feb 21, 2015

We're working on it. We have one potential blocker for the release: #721

zzak added a commit that referenced this pull request Sep 11, 2015

Merge pull request #907 from kgrz/fix-error-with-rack-master
Update ShowExceptions to suit latest rack master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment