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

Fix URLs for multiple media in html_for_tweet() #461

Merged

Conversation

philgyford
Copy link
Contributor

  • I moved the html_for_tweet() tests into their own file.

  • Then fixed an issue with html_for_tweet() displaying media links when there are multiple media items.

    As an example, one tweet that had multiple images was having its URL(s) come out something like:

    wNc6uJklg" rel="external">pic.twitter.com/OwNc6uJklg</a>wNc6uJklg" rel="external">pic.twitter.com/OwNc6uJklg</a>

    rather than

    <a href="https://t.co/OwNc6uJklg" class="twython-media">pic.twitter.com/OwNc6uJklg</a>

We were trying to link to each media item using its
`url`/`expanded_url`. But there is only one of these, shared across
all of a tweet's media items. So attempting to put it in several times,
in the same location, was a bit of a mess!

So it now only puts the `url`/`expanded_url` in once, no matter
how many media items there are.
@coveralls
Copy link

coveralls commented Oct 7, 2017

Coverage Status

Coverage decreased (-0.7%) to 56.498% when pulling a27efd9 on philgyford:fix-media-in-html-for-tweet into 12e6b34 on ryanmcgrath:master.

@philgyford
Copy link
Contributor Author

Hmm, damn, not sure what I've done to reduce coverage. I can't work it out from coveralls.

@michaelhelmick
Copy link
Collaborator

@philgyford looks like api.py increased by 4 lines.

Yours: https://coveralls.io/builds/13616507/source?filename=twython%2Fapi.py
Master: https://coveralls.io/builds/13633835/source?filename=twython%2Fapi.py

You can see lines that are hit and aren't. It shouldn't be too big of a change though. Looks like nothing actually is missed.

@michaelhelmick
Copy link
Collaborator

@philgyford
Copy link
Contributor Author

Whoops, I'll fix it later today/tomorrow. Getting so used to python 3 these days I forget about 2!

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+0.4%) to 57.568% when pulling 5c55aa8 on philgyford:fix-media-in-html-for-tweet into 12e6b34 on ryanmcgrath:master.

@philgyford
Copy link
Contributor Author

I've:

  • Fixed the python 2 issue
  • Added another couple of html_for_tweet() tests to help bump the coverage back up a bit
  • Got rid of some un-needed data from some of the test tweet objects

@philgyford
Copy link
Contributor Author

BTW, I was thinking it would make things a bit easier to follow if the test tweets were nicely-formatted JSON, that was loaded in to use in tests, rather than already being compact python dicts. I was just wary of messing around with things too much. Sound a good idea?

@michaelhelmick
Copy link
Collaborator

@philgyford I think a python file with dicts (that are formatted nicely) or a JSON file would be nice. Either or would achieve the "same" knowledge to edit. config.py looks like it just has a bunch of one line dicts :(

Seems better to have the raw data as JSON, like it comes from the API,
then load it into python objects for each test.
@philgyford
Copy link
Contributor Author

Whoops... do you have any idea how I'd reference a file like tests/tweets/basic.json from within a test, in a way that Travis etc will find it? That's worked for me before, but not this time!

@coveralls
Copy link

coveralls commented Oct 11, 2017

Coverage Status

Coverage increased (+0.4%) to 57.568% when pulling d3f5361 on philgyford:fix-media-in-html-for-tweet into 12e6b34 on ryanmcgrath:master.

@philgyford
Copy link
Contributor Author

Phew, got it :) I think that's me done for now.

@michaelhelmick michaelhelmick merged commit 805590c into ryanmcgrath:master Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants