search

keenlabs / KeenClient-PHP

Official PHP client for the Keen IO API. Build analytics features directly into your PHP apps.
https://keen.io/docs
MIT License
133 stars 57 forks source link

Moving code to PSR-4 #50

Closed cryptiklemur closed 10 years ago

cryptiklemur commented 10 years ago
Q A
Bug Fix?: n
New Feature?: y
BC Breaks?: n
Deprecations?: n
Tests Pass?: y
Fixed Tickets: #48
License: MIT
Doc PR:

Also changes the KeenIOClient::__call() method, so it doesnt override the normal parameters. Makes it so Mockery cant mock it

k-k commented 10 years ago

As for the PS4 change - Is there any issues in the fact that the composer autoload-dev feature was just added, but composer only prompts users to update every 30 days?

Is it worth making this change before that 30 day window?

k-k commented 10 years ago

Not to mention you broke the builds :)

cryptiklemur commented 10 years ago

yeah, we can sit on this until 30 days, doesn't matter. Also... Travis is a jerk.

k-k commented 10 years ago

@aequasi - I agree on the wait, as I think the same thing I warned is why the tests are failing. I believe composer on the travis machine is not current.

You can update the .travis.yml to do a composer self-update before running the tests which I believe will allow the builds to pass successfully.

Otherwise - I agree we sit on this for another 20 odd days before releasing.

cryptiklemur commented 10 years ago

I vote to move the merging of this pull request until April Fools Day! (31 days from merge of autoload-dev)

k-k commented 10 years ago

tests are good. as agreed above - will wait to release. if we get others running into issues with Mockery, we can look at splitting this into separate PRs.

otherwise, thanks for the patch

k-k commented 10 years ago

also, @aequasi ... no :)

edit - I thought you were being a goon, that is actually 30 days. hah.

TomHAnderson commented 10 years ago

I heard composer has pushed their next monthly upgrade out two days so they can finish https://github.com/composer/composer/pull/2696 Curl Transfer so merging today, the 1st may not catalyze the PSR-4 integration you're looking for.

More info at http://www.reddit.com/r/BadPranks/

cryptiklemur commented 10 years ago

pushing back was in regard to the notice that composer gives when the last update was more than a month ago, not sure if that changes your response @TomHAnderson

k-k commented 10 years ago

@TomHAnderson - I'm a little sleep deprived and dealing with a new baby, so I apologize if I missed your point or misread the sarcasm.

The PSR-4 support is live with current composer, but would require a user who has an older version to run the self-update. Users are only prompted by composer to run that update if their current install was more than 30 days old.

Delaying the merge just allowed more time for people to have already updated, or see the notice from composer while running an update or install. Falling on the first was a coincidence.