search

zerodha / phpkiteconnect

The official PHP client library for the Kite Connect trading APIs
MIT License
43 stars 30 forks source link

Making it available as composer library #13

Closed pushpak1300 closed 3 years ago

pushpak1300 commented 3 years ago

Making this available as a composer library for easy maintainability. Please let me know if that's your plan then I can help you guys migrate this to the composer library. I'll be very happy to migrate this to the composer.

vividvilla commented 3 years ago

That will be great 🙂, please feel free to work on the PR.

pushpak1300 commented 3 years ago

Thanks, I will start working on that!

pushpak1300 commented 3 years ago

Hey @vividvilla I have created the composer supported alternative https://github.com/pushpak1300/phpkiteconnect take a look at this and let me know if you think anything needs to change then I can work on the final pull request. Also, I will suggest taking l look at this pattern of defining custom exception to remove unnecessary boilerplate code https://github.com/spatie/color/blob/master/src/Exceptions/InvalidColorValue.php

pushpak1300 commented 3 years ago

I noticed there is a scope of improvements in code by defining value objects and strict types and cleaning some things. I will suggest you look at these. If you want I can work on the code refactor. Also, I will suggest using https://docs.guzzlephp.org/en/stable/ as an HTTP client if using composer also it will help to remove boilerplate code. let me know your thoughts on this.

vividvilla commented 3 years ago

I'm not aware how things are done in PHP nowadays but does adding composer and migrating to guzzlephp forces users to use this lib only via composer? because currently we just ask the users to download the script and include in their project.

I noticed there is a scope of improvements in code by defining value objects and strict types and cleaning some things. I will suggest you look at these. If you want I can work on the code refactor.

This will be really helpful, been meaning to do a refactor but as said previously I don't use PHP anymore on day to day development, so not sure what has changed. Can you post things you have planned for the refactor? we can discuss this here before you start working on it.

@ranjanrak may be you can also get involved.

pushpak1300 commented 3 years ago

Yes If we use guzzlehttp we will have to force users to downloadd this library via composer. But there is a workaround that we can maintain two versions of this SDK kit. one for traditional users and one is for composer users. Also nowadays PHP has improved a lot. We can migrate the codebase to follow PSR-12 standards https://www.php-fig.org/psr/psr-12/. Also where you guys have used an associative array to store different value we can use value objects and we can use type checks for better maintainability of code. Also, I can refactored code a little bit to follow some minor design patterns like ( early return or first fail approach) for better readability. Also, we can add some mockery test to test the codebase,https://github.com/mockery/mockery. I'm not sure about the last one that I will be able to implement that. let me know your thoughts on this. My main motive to move this library to the composer because then I can make support this library for the laravel framework.

vividvilla commented 3 years ago

Yes If we use guzzlehttp we will have to force users to downloadd this library via composer.

Is there any added benefit of using this? because AFAIK inbuilt PHP methods are minimal enough for this lib. But feel free to correct me.

But there is a workaround that we can maintain two versions of this SDK kit.

Lets not do two SDKs, I guess we should just do whatever is the standard now and also distribute in a standard way like we do with pip and npm.

Also nowadays PHP has improved a lot. We can migrate the codebase to follow PSR-12 standards https://www.php-fig.org/psr/psr-12/.

Sure, lets just do it in recent PHP standard.

Also, I can refactored code a little bit to follow some minor design patterns like ( early return or first fail approach) for better readability. Also, we can add some mockery test to test the codebase,https://github.com/mockery/mockery.

That's perfect. Let me know if you need any help, you can approach either me or @ranjanrak. Also If you need we can get you some Kite connect credits to test while you are developing.

pushpak1300 commented 3 years ago

Is there any added benefit of using this? because AFAIK inbuilt PHP methods are minimal enough for this lib. But feel free to correct me.

There are benefits of using a guzzle. But I think it's optional.It depends on us.

Guzzle and cURL are http clients. When you use Guzzle or cURL for the first time you won’t know any difference, but many developers in GitHub prefer Guzzle over curl. Let’s see why they prefer Guzzle. Guzzle uses cURL by default, so many thinks that Guzzle requires cURL. Its not like that Guzzle can use any http client that you want other than cURL like PHP's stream wrapper, sockets, and non-blocking libraries like React. You can reuse code with Guzzle and its more simpler, cleaner, readable and reusable. It also allows you to do async requests in a similar way we do with javascript using promises. It is easier to write unit tests for app and mock the http requests with Guzzle.

Let's not do two SDKs, I guess we should just do whatever is the standard now and also distribute in a standard way like we do with pip and npm.

In the PHP ecosystem, every production project uses a composer to manage the dependencies now a day. So I think Composer is pretty standard in PHP nowadays

Also, I'm not sure I will be needing Kite credits or not but it will be great if I have this right? I don't know how you guys will be approving the credits. Contact me on pushpak1300@gmail.com or Twitter for these or let me know you want anything from my side.

let me know your thoughts on these. so I can start working on the final PR.

vividvilla commented 3 years ago

I did some reading about Guzzle and it seems like the major advantage is in-built support for testing and mocking response which will be useful writing unit tests, so yeah we can go with Guzzle.

Also, I'm not sure I will be needing Kite credits or not but it will be great if I have this right? Also, I'm not sure I will be needing Kite credits or not but it will be great if I have this right?

If you have a Zerodha account then you can with prod data. Credits will be given to your account on developers.kite.trade and will be able to create the app.

pushpak1300 commented 3 years ago

Okay I'll start working on final PR.

My account with kite is with following email pushpak1300@gmail.com. In the meantime you can add the credits so once library is ready I can test it out with kit.

pushpak1300 commented 3 years ago

Thanks, @vividvilla and @ranjanrak for this.