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

Pass token to lookup_users on post_message #583

Merged
merged 3 commits into from
May 27, 2021
Merged

Pass token to lookup_users on post_message #583

merged 3 commits into from
May 27, 2021

Conversation

simonheb
Copy link
Contributor

lookup_user did not work without the parse=FALSE. After adding it, it returned a list also.

lookup_user did not work without the parse=FALSE. After adding it, it returned a list also.
@llrs
Copy link
Member

llrs commented May 24, 2021

Hi, thanks for looking to improve the package, but I'm a bit lost with this PR, you mention lookup_user but the change is on the post_message function.
Could you explain how does this fix lookup_user or what problems did you find with post_message?

@simonheb
Copy link
Contributor Author

Sorry, I was being imprecise. The code calls lookup_user from within post_message. This call was not working, because (1) it was not passing the two options I added, and the return value of lookup_user was enclosed in a list, so that the return values where not being read properly. Maybe the problem is already that lookup_user should not be returning a list (and that the variable of interested was not in ...$user_ids but in ...$id)? But since it was, my changes fixed this.

@llrs
Copy link
Member

llrs commented May 25, 2021

Thank you very much for your explanation, now it is more clear what you expected. There is something I do not understand yet. You mention lookup_user was not working "passing the two options I added,". Which options do you mean?

About the points you mention. (1) Not using the token provided should be fixed. Thanks for pointing it!
However, (2) lookups_users returns a data.frame by default that can be accessed like a list and has the user_id column. It only returns a list (with a data.frame in it with a column id) if it is used with the parse = FALSE argument as you add on this PR. I think the internal call to lookup_users should use the default arguments (but using the token provided) as this makes it easier to adapt to changes on Twitter API.

If you change the PR to only pass the token to lookup_users I'll happily merge it.

Another change that could be made is that if the user provided is already the id post_message shouldn't look up the user id. Perhaps this is what you were trying?. If you want to add a check to user_type and if the results is user_id then skip the call to lookup_users it will be a nice addition.

@simonheb
Copy link
Contributor Author

simonheb commented May 27, 2021

Ok, I agree that we have two separate issues. The token and the parse=TRUE. I agree that where possible, the internal calls should use default options.

The reason I added the parse=FALSE was that there was an error I was getting without it, that was fixed by adding it. Unfortunately I can now not work on replicating the error in a simpler setting and I only recall that it was replacement has [x] rows, data has [y]. I traced it back to the call of lookup_users in my commit and found that adding parse=FALSE fixed the issue for me. I currently don't have the resources to replicate this, but maybe someone who knows the code better can already see how the part that gets executed with parse=TRUE could lead to such an error (when post_message is used with user_ids, at least)?

@simonheb
Copy link
Contributor Author

For now I've reduced this PR to only add the token. But I am still convinced that there's another issue, that will pop up again in the future.

@llrs
Copy link
Member

llrs commented May 27, 2021

Thanks! 🎈
The problem you mention might be due to a known bug on parsing the information from users without any activity (see last comment on #574). I'm (slowly) working on it on #572.

@llrs llrs changed the title fix call to lookup_user Pass token to lookup_users on post_message May 27, 2021
@llrs llrs merged commit 847f365 into ropensci:master May 27, 2021
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.

2 participants