search

DocNow / diffengine

track changes to the news, where news is anything with an RSS feed
MIT License
177 stars 30 forks source link

Twitter integration #71

Closed nahuelhds closed 4 years ago

nahuelhds commented 4 years ago

Hi, @edsu. This one taken a little effort haha.

Additions

At functionality level I've just added the ability to tweet the changes in the same thread and only if the article has at least one diff detected. If the article has no diff, no tweet is generated.

Tests

In order to properly test this, I've taken the Twitter functions into a separate file, so I could mock its functionality when creating the test suite.

I've tested every possible scenario I could think of. Anyway, I'm running this locally within a classic cron on my local machine, in order to ensure that not only the test passes but also works in real life.

When I confirm it's really okay, I'll set the PR ready to be reviewed by you.

About Travis integration

I think you should connect Travis to the diffengine repo and configure it to not let you merge if it doesn't pass so we avoid things like happened in the past PRs. What do you think?

It's very easy to do, really.

I've done myself for my fork so I'm sure that I don't broke it before I go with the PR to this repo. It's passing right now.

Travis-CI-passing

Hope it helps

edsu commented 4 years ago

Thanks for all this work @nahuelhds! I'm glad to see diffengine is breaking out from the monolithic init.py file, and it is going to great to have this thread functionality you have devised merged in. I really appreciate all the efforts you are making to test the twitter integration.

If there is a setting I should change in the .travis.yml let me know. At the moment the tests only run when there has been a change to the master branch? Should I relax that and allow it on branches as well? I believe that GitHub warned me about merging a branch that failed tests earlier, but I tend to merge on the command line.

I'll look forward to hearing if your live testing is working. Is this code running the bots you released recently?

nahuelhds commented 4 years ago

Not really a travis setting at yml level but defining the Travis integration to run the CI cycle on the PRs, so you can see they don't break anything before merging them to master. That's the way I've configure it on my fork, for instance.

nahuelhds commented 4 years ago

I can confirm that it's working fine and creating threads image

@edsu you can review this now.

Next steps:

But I rather merge this first because is a huge change in the code.

edsu commented 4 years ago

Thanks, I will take a look! Isn't Travis already running your PRs?

https://travis-ci.org/github/DocNow/diffengine/pull_requests

Please let me know what knob to turn on GitHub or Travis if you need something changed.

edsu commented 4 years ago

This PR is incredibly thorough and clean @nahuelhds. Thank you for spending the time to clean up, and throughly test the work you did on that initial proof-of-concept branch! This is going to sound overly dramatic, but people like you are the reason why open source ever worked in the first place.

nahuelhds commented 4 years ago

Thank you very much Ed, for this and the tweet today. I have no words. I'm really excited by collaborating with this project. It saved me a lot of work and thinking. So I'm glad I can collaborate with these ideas.

nahuelhds commented 4 years ago

About Travis, I don't see anything related when I open a PR at diffengine repo. Weird, I should see it if it's already integrated, right?