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

WIP: Expand urls in tweets #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bf4
Copy link
Member

@bf4 bf4 commented May 20, 2013

Needs tests and redesign, but otherwise works

@chrishunt
Copy link
Contributor

Please rebase so we only have c6429dcf 8cd5936d 673b25a4

@bf4
Copy link
Member Author

bf4 commented Jun 2, 2013

Testing this requires the previous pull requests... before merging, ensure you can rake refresh_tweets and load a page in your browser


describe ExpandUrl do

it 'should be tested'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Pretty please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. I was thinking of also moving the expansion to the tweet processing leve, but since that would be destructive, was considering adding an 'original tweet text' field. Unrelated, I want to add a 'context' field that represents the search term the tweet was found from, then... eventually extract most of this as an engine.. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm rewriting this to add a tweet_display_text field that is populated at import time.. should I continue to update this pull request?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make it easy to merge and I'll do so.. Keep adding tests and I don't mind merging at a faster rate, given the feature is not going to bloat the code base -- aka, it is derived from something needed, with a reason that outweighs the expense of maintenance moving forward..

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.

3 participants