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

Make the class usable through Composer and add a small test #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

j3j5
Copy link

@j3j5 j3j5 commented Apr 12, 2015

I've moved the class to a folder structure following PSR-0 so people can use this class through composer. Feel free to add more info on the composer.json and then we can create the package on packagist.org so people can download and use this great library.

@marcushat
Copy link
Owner

Awesome, would love to see this on Packagist!
Can you change the test to something besides Google though because they sometimes block requests above a certain rate, maybe a script someone can run on their own server incase they want to test LOTS of concurrent requests. Everything else looks good though so I'll go ahead and merge it after this.

@j3j5
Copy link
Author

j3j5 commented Apr 13, 2015

I've changed the test to use some free currency converter API. They don't seem to complain with the few tests I did. Also, I already created the package on packagist, I'll update the repo and transfer you the ownership if you want as soon as the code is merged here. Also, feel free to add more info about you on the composer.json file.

@marcushat
Copy link
Owner

Okay, cool! I'm gonna put all composer files in their own directory instead of the main one though, since not everyone uses composer(I don't most of the time).
Also cleaned up the test script a little, but haven't tested it though. I'll merge it when I do and regenerate the hash for "composer.lock"
And thanks for contributing.

@j3j5
Copy link
Author

j3j5 commented Apr 14, 2015

Awesome, I just rebased the branch against your latest version (including the fix for #9 ), let me know if you need anything else and thanks to you on the first place for building this class.

@half2me
Copy link

half2me commented Feb 9, 2016

This should get merged

@marcushat
Copy link
Owner

I agree. I need to update the composer files and tests first though. I'll merge it by next week.

@j3j5
Copy link
Author

j3j5 commented Feb 10, 2016

I can try to update the branch with your latest changes, I'll see if I can have time this week. Once you do, I'll update packagist as well to point to your repo.

@half2me
Copy link

half2me commented Feb 10, 2016

I forked this project and turned it into something a little different. I would much apreciate comments/ideas.
https://github.com/half2me/curlx

Let me know what you think about the new interface

@marcushat
Copy link
Owner

Hmm, I definitely like the idea of being able to append stuff(options, headers, etc...) to the ones already set!
...but I think using a magic setter function for this is a bit confusing. I think just adding a few methods would be better.

Gonna use your fork as inspiration and split up the individual requests and the main controller in an upcoming version so we can do this in the simplest way possible.

@half2me
Copy link

half2me commented Feb 10, 2016

@marcushat you don't have to use the magic functions, they just provide a nice way of doing $object->value without having to use getters and setters all the time.

But if you prefer getters/setters, they are all available, and documented in the Request interface

@marcushat
Copy link
Owner

marcushat commented Feb 10, 2016

Alright cool. I'll add all this functionality to the next version, v3.0(or some other later version)! Coming Soon!

@j3j5 j3j5 force-pushed the master branch 3 times, most recently from deae2b8 to a61982c Compare February 21, 2017 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants