Truncated display_urls raise an error #459

Merged
merged 4 commits into from Oct 2, 2013

Projects

None yet

3 participants

@michaelherold
Contributor

The way that Twitter truncates their URL entity display_urls is causing an error at lib/twitter/base.rb:57, which uses the ::URI.parse method to parse the display_urls. This becomes a problem when printing out HTML versions of the tweets and using the entity location information given in the Tweet entity.

To see an example of this, pull tweet 381940500032258049 and check out the result. An example of this is shown at this gist.

According to the Twitter Entity docs, display_urls are "[n]ot a URL but a string to display instead of the media URL," so I don't think they should actually be parsed as URIs, but instead just made into strings.

This pull request contains a failing spec in the first commit. The second commit is a set of changes that makes display_uris into Strings, along with all the spec changes that that entails. It's a pretty heavy commit, so I don't know that it's the preferred way to go about it, but I wanted to give you an attempt at it.

@sferik
Owner
sferik commented Oct 2, 2013

Hi Michael,

Thanks for bringing this issue to my attention. I like what you’ve attempted here but the patch doesn’t apply cleanly to the master branch. Could you please pull down the latest master and rebase your changes so they can be merged without conflict?

Thanks!

@coveralls

Coverage Status

Coverage decreased (-35.84%) when pulling 9eacf39 on michaelherold:display-uri-issue into 6e0a09c on sferik:master.

@michaelherold
Contributor

Whoops, my bad. Always seem to forget a step.

I've rebased, so they should merge without trouble. I also DRYed up the way I made the fix by putting logic into the uri_attr_reader method instead of defining a mostly copy-and-paste display_uri_attr_reader method. Makes it so you don't have to remember the existence of a new method for uri attrs.

@michaelherold
Contributor

Travis told me I forgot something else, so I had to set encoding on the uri_spec.rb file because of the unicode character in the sample display_url. Hopefully this fixes everything.

@coveralls

Coverage Status

Coverage increased (+0.0%) when pulling 4dff29b on michaelherold:display-uri-issue into 6e0a09c on sferik:master.

@sferik sferik merged commit 9e9baa7 into sferik:master Oct 2, 2013

1 check passed

default The Travis CI build passed
Details
@sferik
Owner
sferik commented Oct 2, 2013

Thanks!

@sferik sferik added a commit that referenced this pull request Oct 2, 2013
@sferik Cleanup #459 f5ffaca
@sferik sferik added a commit that referenced this pull request Oct 2, 2013
@sferik Cleanup #459 f1f9087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment