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

Move all code to a class, add unit tests, handle multiple pages of milestones and labels #4

Merged
merged 19 commits into from
Jul 2, 2019

Conversation

williamdes
Copy link
Contributor

@williamdes williamdes commented Apr 24, 2019

  • Moved all code to a class
  • Added unit tests
  • Fix to handle multiple pages of milestones and labels (Multiple pages of milestones #3)
  • Added some properties into composer.json
  • Upgraded dependencies
  • Improved code by removing hard coded sql insert statements
  • Added declaration for github-to-mysql into composer.json binary section, see: 8a0980e

To review, you can review commit by commit

@williamdes
Copy link
Contributor Author

Tested using ./github-to-mysql sync --since-forever phpmyadmin/phpmyadmin since this command would not succeed before this PR, see: #3

@williamdes
Copy link
Contributor Author

@mnapoli Can you please review my work ?

@mnapoli
Copy link
Contributor

mnapoli commented Apr 25, 2019

@williamdes I have been removed from this repository and I have no longer rights to maintain it. I don't know if this project is maintained, an option might be to use your own fork.

@williamdes
Copy link
Contributor Author

cc @jeremy-wizaplace @luxifer

@luxifer
Copy link
Contributor

luxifer commented Apr 25, 2019

Neither do I

@williamdes
Copy link
Contributor Author

@anthHugo?

@steevanb
Copy link

We are waiting our CTO to know if we continue to maintain this repository :)

app/data.php Outdated
use GuzzleHttp\Client;
use GuzzleHttp\Exception\ClientException;

class data {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow a few principles here:

  • uppercase names for classes
  • separation of concerns
  • useful constructors

I suggest splitting this class in 2, one for database write operations, and one for http fetch operations.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@wizacedric
Copy link

@williamdes thanks for your work on your PR. These are some very good improvements, but I think you can go a bit further 🚀 .

composer.json Outdated Show resolved Hide resolved
@williamdes
Copy link
Contributor Author

@wizacedric I improved this PR using some of your principles

I suggest splitting this class in 2, one for database write operations, and one for http fetch operations.

I will maybe do some work on this

@williamdes
Copy link
Contributor Author

@wizacedric Would you accept to merge this PR ?

@williamdes
Copy link
Contributor Author

@wizagael can you please review my PR?

@williamdes
Copy link
Contributor Author

cc @wizacedric ..
I invested a lot of my free time, please review or merge

@wizacedric wizacedric merged commit 6bd1f1e into wizaplace:master Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants