-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add options to ignore retweets and boosts (reblogs) #14
Conversation
3b94672
to
1bf0658
Compare
93edd36
to
425978f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, I think the changes make sense!
At first I thought we should filter in sync.rs, but of course if we can prevent fetching retweets/reblogs that is even better. The only problem is that we have no test coverage now.
Also we could run into problems with this: What happens if the Mastodon account has 50 reblogs and no original toot? Then the list of Mastodon is empty and there is no latest toot to compare? Then we would try to double post to Mastodon?
Do we run into feedback loops with this when people change their configuration to ignore retweets/reblogs suddenly? At least the double posting prevention should mitigate this somewhat and maybe we don't care?
Yeah, I am worried about the lack of test coverage too. Initially, I thought there are not tests for the current happy path (syncing retweets/reblogs), because as a new Rust developer coming from Node.js land, I didn't realize I should look for the tests inside the source files. (In Node.js, tests have their own files.) Now I see there is pretty decent test suite in
Interesting. I didn't look into the details of the sync algorithm, I just tried the simplest thing that seemed to work for me :) Thank you for pointing out edge cases to consider!
I am afraid I don't understand what do you mean by a feedback loop and double posting prevention. Would this problem go away if we always fetch retweets/reblogs and filter them out inside the sync algorithm, because that way the sync algorithm always receives the same input data, regardless of whether retweets/reblogs should or should not be synced? Based on your comments, it seems to me that the easiest (and possibly the most robust) way forward is to rework my pull request to deal with reblogs/retweets inside the sync algorithm. Could you @klausi please confirm? This is a fun side project for me and I have very limited time budget to work on it, I think it will take me a week or two until I have a new version for review. |
Confirmed, I agree that we should do this in sync.rs where we can also tests this. Not fully ideal as I said, but a good enough start that should be safe. No time pressure, thanks a lot for your contribution ❤️ |
425978f
to
c4dfc36
Compare
Add two new config options allowing users to disable syncing of Twitter retweets and Mastodon reblogs (boosts). Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
c4dfc36
to
6e215a6
Compare
@klausi thank you for a very constructive feedback 👍 I reworked the pull request to implement Personally, I would like the following behavior for syncing quote tweets:
Consider the following quote tweet: https://twitter.com/bajtos/status/1252469674681544704 At the moment,
What I would like to see instead (conceptually, I am not sure how attachments/links work in Mastodon):
I briefly considered implementing this behavior by default when Anyhow, I wrote all of the above to give you more context about what I am looking for, to better explain why I don't care that much about |
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
I just recently implemented the quote tweet behavior because I found it really annoying to have twitter links on Mastodon. There is enough post character space on Mastodon to also include the quote tweet then the user does not have to click to get to the content Twitter. Of course I'm open to move that behavior under a configurable flag, we can discuss in a new issue :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, test cases look sufficient! I will merge this now and follow-up with small default value changes in the README.
Thanks for your contribution and feel free to come back any time with further improvements!
@@ -37,6 +37,8 @@ Enable automatic status/favourite deletion with config options. Example: | |||
delete_older_statuses = true | |||
# Delete Mastodon favourites that are older than 90 days | |||
delete_older_favs = true | |||
# Do not sync reblogs (boosts) | |||
sync_reblogs = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value should be true also in the docs to have the current behavior.
@@ -56,6 +58,8 @@ user_name = "example" | |||
delete_older_statuses = true | |||
# Delete Twitter likes that are older than 90 days | |||
delete_older_favs = true | |||
# Do not sync retweets | |||
sync_retweets = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default value should be true also in the docs to have the current behavior.
@@ -460,7 +484,7 @@ UNLISTED 🔓 ✅ Tagged people | |||
|
|||
let tweets = vec![tweet]; | |||
let statuses = vec![status]; | |||
let posts = determine_posts(&statuses, &tweets); | |||
let posts = determine_posts(&statuses, &tweets, &DEFAULT_SYNC_OPTIONS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit annoying that we have this API change. I thought about doing a bit of compatibility dance and keep determine_posts() with the same function signature. Then add determine_posts_with_config() which can be called by main(). But probably that is not worth it and just adds one layer of indirection, and refactoring tests like this is fine. mastodon-twitter-sync is not a library, so we can mess up function signatures all we want :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is annoying. On the other hand, now that we have made this change, adding more sync options should be much easier in the future.
Makes sense, I see how Twitter links on Mastodon can be annoying. I guess my point of view is that I don't want to (re)post text written by other users and want to treat quoted tweets the same way other links are treated (e.g. when tweeting about a blog post published elsewhere).
Sounds great 👍 |
Thank you @klausi for landing my pull request! ❤️ |
Introduce two new config options:
sync_retweets = false
to exclude retweets when sychronizing from Twitter to Mastodonignore_retweets
sync_reblogs = false
to exclude boosts (reblogs) when synchronizing from Mastodon to Twitterignore_boosts
Resolve #11
Caveat:mammut
does not supportexclude_reblogs
option introduced to Mastodon API in Dec 2018 via mastodon/mastodon#9640, we have to filter out reblogs locally at client side. Mammut is no longer maintained and recomment users to migrateelefren
. I opened a feature request askingelefren
to add support forexclude_reblogs
, see https://github.com/pwoolcoc/elefren/issues/134.Discussion point: considering that Mastodon uses the term "reblogs" instead of "boosts", should we perhaps do the same and call the new parameterignore_reblogs
?