Till Klampaeckel [1970-01-01 00:33 UTC] Original vote: 1
Conditional vote: yes
Comment on vote: Looks great so far, also kudos on the tests.
My additions/requests which make this conditional:
1) Always use type hinting (e.g. for the consumer, etc.) (nice to have)
2) Your class names are off, e.g. class Services_TwippleUploader should be Services_OAuthUploader_TwippleUploader (required per PEAR cs)
E.g. pear package-validate:
Warning: in YfrogTest.php: class "Services_YfrogUploaderTest" not prefixed with package name "Services_OAuthUploader"
Warning: in TwitpicTest.php: class "Services_TwitpicUploaderTest" not prefixed with package name "Services_OAuthUploader"
Warning: in TwitgooTest.php: class "Services_TwitgooUploaderTest" not prefixed with package name "Services_OAuthUploader"
Warning: in TwippleTest.php: class "Services_TwippleUploaderTest" not prefixed with package name "Services_OAuthUploader"
Warning: in PlixiTest.php: class "Services_PlixiUploaderTest" not prefixed with package name "Services_OAuthUploader"
Warning: in ImglyTest.php: class "Services_ImglyUploaderTest" not prefixed with package name "Services_OAuthUploader"
Warning: in YfrogUploader.php: class "Services_YfrogUploader" not prefixed with package name "Services_OAuthUploader"
Warning: in TwitpicUploader.php: class "Services_TwitpicUploader" not prefixed with package name "Services_OAuthUploader"
Warning: in TwitgooUploader.php: class "Services_TwitgooUploader" not prefixed with package name "Services_OAuthUploader"
Warning: in TwippleUploader.php: class "Services_TwippleUploader" not prefixed with package name "Services_OAuthUploader"
Warning: in PlixiUploader.php: class "Services_PlixiUploader" not prefixed with package name "Services_OAuthUploader"
Warning: in ImglyUploader.php: class "Services_ImglyUploader" not prefixed with package name "Services_OAuthUploader"
3) If a __construct doesn't do anything but call parent::__construct(), you don't need to implement it at all. (nice to have ;-))
4) fix your package.xml:
sudo pear install package.xml
ERROR: file ./Services/OAuthUploader/Exception.php does not exist
I'm guessing this is because the files are in 'src', I just added <dir name="src"> around it and it seemed to have worked, but then the problem is that it installs into "php_dir/src/YOURFILES".
The workaround (and anyone please correct me if I am wrong) is either to drop the "src" directory completely or to transform the paths on install.
5) Instead of hard-coding "0.1.0" into the files, use "Release: @package_version@" and add a replacement rule to package.xml (nice to have):
http://pear.php.net/manual/en/guide.developers.package2.tasks.php
6) Improve tests: Down the road, I'd add a config with credentials for the services. If no config file/credentials is/are provided for a service, skip the test (instead of failing it). In this regard -- it might also make sense to mock the services so the tests always run no matter if something is down or not. (nice to have)
Anyway, this looks really great! Looking forward to having it in PEAR.
Till
Reviewed: cursory, deep, test
Christian Weiske [1970-01-01 00:33 UTC] Original vote: -1
Conditional vote: no
Comment on vote: I'd really like to see the package in pear, but you did not follow any of the rules and called for votes without requesting any comments. The voting phase is not for getting comments about your code as Till provided them.
If you re-propose the package and discuss with us before calling for votes, I'll probably give you my +1, but not now.
Reviewed: cursory
takumi taka [2011-01-08 11:03 UTC] >developers
thanks rollback.
>till
fix 1..5 problems.
https://github.com/withgod/Services_OAuthUploader/commits/master
6 is contain these config with package.
>cweiske & others.
please let me advice :)
Christian Weiske [2011-01-15 23:05 UTC] - Run PHP_CodeSniffer on your code
- why are there twitter constants in the main class?
- class variables: either put the description above the @var, or behind it - not both
- please describe your method parameters textually
- no "<?", only "<?php"
takumi taka [2011-01-24 08:59 UTC] - Run PHP_CodeSniffer on your code phpcs error "Missing @return tag in function comment" but these method don't have return value.
http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.return.pkg.html
void return method exmaple is not documented.
i referred http://stackoverflow.com/questions/2061550/phpdoc-return-void-necessary
plz consider that i'm just a norvice php programmer.
- why are there twitter constants in the main class?
other classes are using them.
No better here, it's useful to present here but...
- please describe your method parameters textually
sry i can't understand that problem.
ask on irc.
- no "<?", only "<?php"
- class variables: either put the description above the @var, or behind it - not both
putting the description before / after the @var did not solve the issue
thanks for taking time with my poor English and thanks in advance.
Till Klampaeckel [2011-02-25 15:28 UTC] Hi,
just commenting over here as well. I just send you a pull request:
https://github.com/withgod/Services_OAuthUploader/pull/1
I personally think the code is fit for a release, there is one minor thing though. ;-)
I think part of the mis-understanding what this class does is that it's not a generic OAuth uploader, but a class that allows uploading pictures to services which only allow people to login/signup using OAuth via Twitter.
@Cweiske: That's also why the twitter constants are in the main class. All these services rely for authentication on Twitter, so that makes sense.
I think namewise, I'd probably do something like, Services_Twitter_OAuthUploader - what do you guys think?
Otherwise, while I think there's (always) room for improvement, the implementation is fit for a vote.
Till
Michael Gauthier [2011-02-25 20:44 UTC] Looks great. My feedback:
1.) Is this package specific to twitter? The main class and subclasses use TWITTER_VERIFY_CREDENTIALS_JSON, can the Twitter stuff be made optional or is OAuth uploading specific to twitter?
2.) exception does not need to extend pear_exception
3.) you should clone the http_request object before calling send(), use it as a template object. This will prevent the state of previous calls from breaking subsequent calls.
4.) consider using http_build_query to build post data
5.) use $class instead of $clazz
6.) if json_parse returns null (bad response data), calling property_exists on the result will cause a php warning
7.) docs for pre and postUpload in concrete classes are incomplete. They should at least link to the parent method docs. Parent method docs don't say what the methods do.
8.) add a setRequest method that lets you set the HTTP_Request2 object to use after constructing the object
9.) add a setConsumer method that lets you set the HTTP_OAuth_Consumer object to use after constructing the object
10.) it doesn't matter to me, but you can probably lower the required php version to 5.1.2
takumi taka [2011-06-17 08:15 UTC] sorry delayed response.
#i've been busy with work trouble(earthquake..) and got married :)
>cweiske, till, gauther
thanks feedback sir.
implemented, fix your feedbacks.
https://github.com/withgod/Services_Twitter_Uploader/commits/master
i planning to 1week later 'call for votes' status.
|
