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

Twitter integration #71

Merged
merged 23 commits into from
May 1, 2020
Merged

Conversation

nahuelhds
Copy link
Contributor

@nahuelhds nahuelhds commented Apr 30, 2020

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
Copy link
Member

edsu commented Apr 30, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@edsu you can review this now.

Next steps:

  • Optional text for the different types of changes: title, url, summary. E.g.: a tweet saying "Changes in the title and the url" and so.
  • Configurable logging directly to the console (useful when running from the cli and debugging (not depending on the log file only)
  • Configurable database engine (sqlite, postgres)

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

@nahuelhds nahuelhds marked this pull request as ready for review April 30, 2020 20:31
@nahuelhds nahuelhds marked this pull request as ready for review April 30, 2020 20:33
@edsu
Copy link
Member

edsu commented May 1, 2020

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
Copy link
Member

edsu commented May 1, 2020

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.

@edsu edsu merged commit 204ce2d into DocNow:master May 1, 2020
@nahuelhds
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

@nahuelhds nahuelhds deleted the feature/tweets-as-thread branch May 1, 2020 15:26
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