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

Better nick handling in 'repall' #90

Merged
merged 3 commits into from Mar 8, 2015
Merged

Better nick handling in 'repall' #90

merged 3 commits into from Mar 8, 2015

Conversation

Tenzer
Copy link
Contributor

@Tenzer Tenzer commented Feb 26, 2015

I wrote this reply by using the repall command, and as you can see it mentioned myself first. I had a look at the code behind it and made a few improvements:

  1. Make sure the person who wrote the tweet we are replying to is mentioned first (unless it's the RS user).
  2. Make sure we don't mention the RS user.
  3. Make sure the same user isn't mentioned multiple times.

I also switched the previous nick parsing to use the entities list of the tweet. Here's a link to the documentation for that.

- Always have the owner of the tweet responding to first in the response
- Never include the user's nick in the list
- Make sure we don't have duplicate nicks

All with the added bonus of avoiding regexing due to Twitter's entities list
@Tenzer
Copy link
Contributor Author

Tenzer commented Feb 26, 2015

Thinking about it, there's only one case where these changes won't "do the right thing", that is if you use repall on your own tweet where you don't mention anybody else. In that case the tweet would be prefixed with the white space which is inserted between the list of nicks (which would be empty) and the message.

Perhaps it would be better to move the filtering of the user's own nick into the for-loop, so the person you are responding to always will be mentioned first in the reply, regardless of it being yourself or not. What do you think?

@orakaro
Copy link
Owner

orakaro commented Feb 27, 2015

Hi @Tenzer Thanks for contributing 👍 I can see why this should be improved, but how to fix is really hard. Can you show me how to move the filter ?

@Tenzer
Copy link
Contributor Author

Tenzer commented Feb 27, 2015

Sure, I have done it in commit 97def8f. I copied the code styling used in trend() in order to avoid having a too long line with the if. If you have any comments for this, let me know.

@orakaro
Copy link
Owner

orakaro commented Mar 2, 2015

Thanks !! I will test this for a while before merge

@Tenzer
Copy link
Contributor Author

Tenzer commented Mar 2, 2015

I'll do the same. If it turns out to work fine, I think it would make sense to look through the code and see if there are other places where it could benefit from using entities instead of parsing items from the tweet text.

@Tenzer
Copy link
Contributor Author

Tenzer commented Mar 4, 2015

I just pushed an extra commit to the branch, it corrects a mistake I made where I try to iterate over the tweet module instead of the original_tweet variable, so it didn't work before.

orakaro pushed a commit that referenced this pull request Mar 8, 2015
Better nick handling in 'repall'
@orakaro orakaro merged commit 47b377c into orakaro:master Mar 8, 2015
@Tenzer Tenzer deleted the better-repall branch March 8, 2015 18:42
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

2 participants