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

Adding blocking and unblocking #593

Merged
merged 21 commits into from
May 3, 2022
Merged

Adding blocking and unblocking #593

merged 21 commits into from
May 3, 2022

Conversation

simonheb
Copy link
Contributor

I was missing blocking and unblocking functionality.

Since muting was inside post_follow, I added blocking (and unblocking) to the same file. It's tested and worked for me.

@llrs
Copy link
Member

llrs commented Jul 12, 2021

Thanks! Not sure about adding new features with API v1, but it will be useful as a reminder. Could you add a test (add some existing "random" user). The code change seems minimal and without major issues (I think you need to document again the package though, as it fails on the CI checks due to this).

@hadley
Copy link
Member

hadley commented Jul 12, 2021

I'm uncertain if its a good idea to continue to add even more features to this already large function. Maybe it would be better to have new functions?

@llrs
Copy link
Member

llrs commented Jul 12, 2021

Yes, I had drafted a comment myself regarding this. However, I also hesitate to create new functions until #572 isn't merged and renaming all functions following previous discussions (I don't find the issue/PR were we discussed this now).

@simonheb
Copy link
Contributor Author

not sure I understand the comment about v1? what is your preferred way to deal with functionally that does not yet exist?

I appreciate the feedback about not bloating this one function. Since I do not know what the general plans are regarding the overall structure I will not push for further additions.

@simonheb
Copy link
Contributor Author

I've added the tests and documented it. Should work now.

@llrs
Copy link
Member

llrs commented Jul 13, 2021

There is a new API v2, currently rtweet is using API v1. There are many differences between the way to call data, the endpoints and the data returned.

Currently my efforts are to set up the package in better shape for adding support for API v2 as Twitter is clearly moving to use and update only API v2. I don't know how long it will take but API v1 will at some point be deprecated (years probably). Adding features is great, specially if they require few time and checks on my side. But this is also depends on how are they implemented and in relation to other functions. Just don't add features because they are cool but because you use them (or want to use them) and we'll find a way to get them included.

On this particular case it is true that post_follow is already doing many different so I think it is better to move it to its own function block_user(user, unblock = FALSE).

@simonheb
Copy link
Contributor Author

But afaik the API v2 is not yet accessible outside of the early access program. Or am I misinterpreting this?

Yes, I implemented the blocking because I needed it for my app. I will move this into a new function like you suggested.

@simonheb
Copy link
Contributor Author

ok, all [un]blocking functionality is now in R/post-block.R, in a separate function, as @hadley suggested.

@llrs
Copy link
Member

llrs commented Jul 13, 2021

The early access program is quite open to developers and lately they are doing events and explaining how to work with academictwitteR.
The package received a comment from one of the developers of Twitter around a year ago (see #402), so it is acceptable to provide support and move to the new API.

@simonheb
Copy link
Contributor Author

simonheb commented Jul 13, 2021

Ok, interesting. I've been fiddling around with some code (e.g. to add support for fetching alt-text/captions for tweets with images, as I am using rtweet to run the bot @CaptionClerk). I was considering merging this functionality into rtweet, but maybe it's better if I wait until this has moved to the v2. I could also try to help with this, just give me a hint on where my contributions could be useful.

@simonheb simonheb changed the title Adds blocking and unblocking Adding blocking and unblocking Jul 13, 2021
@llrs
Copy link
Member

llrs commented Jul 13, 2021

@simonheb Maybe you can tackle #566 (I had a PR on the wrong direction #567)

@hadley
Copy link
Member

hadley commented Jul 13, 2021

Should these be user_block() and user_unblock()? Or block_user() and unblock_user()?

@simonheb
Copy link
Contributor Author

I agree that block_user() and unblock_user() are more intuitive. Will change this.

@llrs
Copy link
Member

llrs commented Jul 19, 2021

About the naming convention we discussed this on #480. I think user_block() and user_unblock() makes more sense and it is following the first suggestion from this comment and the first suggestion from Hadley (sorry you need to rename this functions again).

@simonheb
Copy link
Contributor Author

ok no worries, fair point. I was not aware of #480. would it additionally make sense to rename the file post-block.r to something else?

@llrs
Copy link
Member

llrs commented Jul 21, 2021

Oh, I hadn't tough about renaming files, probably it would be nice to have matching files (so user-block.R). But don't worry if you don't, soon I'll start a new PR to rename all functions and then I'll see if renaming files makes sense.

@simonheb
Copy link
Contributor Author

Okay, then I wont rename this. Probably best to keep this separate

@ronnicolasp
Copy link

Please allow me to join in! Can I contribute?

@llrs
Copy link
Member

llrs commented Jan 27, 2022

Hi @ronnicolasp, what do you want to join? What do you want to contribute?

@llrs
Copy link
Member

llrs commented Mar 26, 2022

I updated the PR with recent changes, added some tests and changed the user on the examples.
I'm still not decided about the names, the point on #480 about tab completion is compelling (user_block, user_block_destroy) but at the same time I wish it felt more natural to the users (user_block, user_unblock).

@llrs llrs merged commit 3d49265 into ropensci:master May 3, 2022
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.

4 participants