Details
-
Type:
Task
-
Status:
Resolved
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: 1.5.0
-
Component/s: Zend_Service_Technorati
-
Labels:None
-
Fix Version Priority:Should Have
Description
Once unit testing for each file of the component reaches 80% LOC coverage and there exists satisfactory DocBook documentation for the APIs, the Zend_Service_Technorati component needs to be promoted to core (i.e., moved to trunk with unit tests and documentation integrated).
Attachments
1. |
Class docblocks |  |
Resolved
|
Simone Carletti | |
2. |
Clear/Fix all @todo from Unit tests | |
Resolved
|
Simone Carletti | |
3. |
Fix all xpath expression @todo | |
Resolved
|
Simone Carletti |
Activity
@ Darby
With r7263, I feel the library is ready.
I've included 99% of unit tests and full documentation.
What do you think? 
Great! I'll review this today and post again here. Thanks for these contributions!
Sorry, Simone, for the delay in getting back with you... I haven't forgotten! I plan to take a look first thing tomorrow and post again here.
I plan to take a look first thing tomorrow and post again here. Okay, maybe I should not have second-guessed myself. Or am I now doubting my doubts? 
I ran the following script on my machine:
<?php
error_reporting(E_ALL | E_STRICT);
set_error_handler('errorHandler');
function errorHandler($errno, $errstr, $errfile, $errline, $errcontext)
{
echo 'errorHandler() called';
throw new Exception($errstr, $errno);
}
function foo(stdClass $obj)
{
echo 'foo';
}
try {
foo('scalar');
} catch (Exception $e) {
echo 'Caught exception';
}
Results:
Fatal error: Argument 1 passed to foo() must be an object of class stdClass, called in C:\wamp\www\test.php on line 19 and defined in C:\wamp\www\test.php on line 13
I'm pretty sure that this means that at least on my machine running 5.1.4, the failure to meet the type hint does not result in a catchable fatal error.
Or am I now doubting my doubts? I ran the following script on my machine:
<?php
error_reporting(E_ALL | E_STRICT);
set_error_handler('errorHandler');
function errorHandler($errno, $errstr, $errfile, $errline, $errcontext)
{
echo 'errorHandler() called';
throw new Exception($errstr, $errno);
}
function foo(stdClass $obj)
{
echo 'foo';
}
try {
foo('scalar');
} catch (Exception $e) {
echo 'Caught exception';
}
I've just installed xdebug and run the test coverage report.
Utils.php includes 3 methods defined but never implemented, this is the reason.
They are utility methods useful for DRY development, perhaps I have to comment them for now.
ResultSet.php is not a class intended to be used alone, as Result.php.
It should be tested directly from parent classes, hovewer I'm going to have a look at it.
TagsResultSet.php has 2 methods, one is the constructor.
The second method is missing a test method, this is why the class has about 50% of code coverage.
I'm going to fix missing tests in the next hours.
Any other overall feedback?
Hi Darby,
happy new Year!
I saw your changes and I agree with all of them except one I'd like to ask you for more details.
Here you removed the second piece of code with the explanation
- removed code impossible to execute from setUriHttp() method
This is true right now, but according to Shahar's Zend_Uri improvements proposal Zend_Uri is going to support additional schemas.
Even if this is just a proposal, is reasonable to suppose that the future of Zend_Uri is open to new schemas thus the second check was created to be sure the result of
is a valid instance of a Zend_Uri_Http and not, for example, Zend_Uri_Mailto.
Unit tests are good enough to point out the error as soon as Zend_Uri will support more protocols, but restoring the control will prevent a 'last minute fix'.
What do you think?
About @todos you are right, there's an high number of @todo but just because I usually first develop the basic feature and I like to add notes to remember the feature might be improved in a second time.
I mean, @todos don't mean the code doesn't work, only it could have a better interaction with the environment (for example, @todo Zend_Uri_Http).
I'm going to review the code addressing first all todos that may fit the classification 'should have', then I will create a new issue for any 'nice to have' as I did, for example, for ZF-2350.
I saw your changes and I agree with all of them except one I'd like to ask you for more details.
Here you removed the second piece of code with the explanation
This is true right now, but according to Shahar's Zend_Uri improvements proposal Zend_Uri is going to support additional schemas. Even if this is just a proposal, is reasonable to suppose that the future of Zend_Uri is open to new schemas thus the second check was created to be sure the result of
- removed code impossible to execute from setUriHttp() method
I'm going to review the code addressing first all todos that may fit the classification 'should have', then I will create a new issue for any 'nice to have' as I did, for example, for I saw your changes and I agree with all of them except one I'd like to ask you for more details. Here you removed the second piece of code...
Oops, I had in mind that the check was against Zend_Uri and not Zend_Uri_Http. Sorry I misunderstood. Please feel free to resurrect that section of code, or if you'd like, I can do it.
I've cleared/fixed all reasonable @todos, added more unit tests focused on SeekableIterator interface.
Awesome; thanks!
Just a final question: should Zend_Service_Technorati_ResultSet#getXML() be renamed to Zend_Service_Technorati_ResultSet#getXml(), isn't it?
Yes, it should be getXml() according to the naming conventions in the coding standards.
I'll perform what I hope might be a final review as soon as possible. I also suggest contacting the fw-webservices mailing list (and forwarding to fw-general, perhaps) to give the code a whirl to test it and provide feedback, to catch any lingering API issues, etc. that should be addressed prior to release with the framework core.
I saw your changes and I agree with all of them except one I'd like to ask you for more details. Here you removed the second piece of code...Oops, I had in mind that the check was against Zend_Uri and not Zend_Uri_Http. Sorry I misunderstood. Please feel free to resurrect that section of code, or if you'd like, I can do it.
I've cleared/fixed all reasonable @todos, added more unit tests focused on SeekableIterator interface.Awesome; thanks!
Just a final question: should Zend_Service_Technorati_ResultSet#getXML() be renamed to Zend_Service_Technorati_ResultSet#getXml(), isn't it?Yes, it should be getXml() according to the naming conventions in the coding standards. I'll perform what I hope might be a final review as soon as possible. I also suggest contacting the fw-webservices mailing list (and forwarding to fw-general, perhaps) to give the code a whirl to test it and provide feedback, to catch any lingering API issues, etc. that should be addressed prior to release with the framework core.
Hi Darby,
I was wondering if you are going to move the component to core before the upcoming code freeze.
Yes, it seems as though something has changed with Zend_Date, as I have never seen such errors in the Zend_Service_Technorati tests until now.
Probably it would be a simple matter to provide the date format, as Thomas suggests, to get the tests passing again.
I am concerned, however, that there has been a backward compatibility break with Zend_Date, inasmuch as people have been depending on certain behavior that is now broken. 
That said, perhaps it is not a backward compatibility break in the sense that perhaps the usage within Zend_Service_Technorati was broken, non-standard, not documented, or otherwise unsupported behavior. Thomas could probably shed some light on this. All things considered, I don't really understand why Zend_Date::isDate() cannot grok the same date value that new Zend_Date() supports. Strange, indeed.
That said, perhaps it is not a backward compatibility break in the sense that perhaps the usage within Zend_Service_Technorati was broken, non-standard, not documented, or otherwise unsupported behavior. Thomas could probably shed some light on this. All things considered, I don't really understand why Zend_Date::isDate() cannot grok the same date value that new Zend_Date() supports. Strange, indeed. Hi Simone,
It looks as though Zend_Service_Technorati is ready to be moved to core. Are you aware of anything that should be done beforehand?
Would you like to migrate the component, or would you like me to do it on your behalf?
Thanks for these excellent contributions!
Hi Darby,
according to official documentation only a core contributor could promote a component to core this is why I never moved a single bit.
By the way, if you want I can move it and left you free for other activities.
I suppose I only need to
- svn move incubator/library incubator/test to /library /test
- promote documentation
- update docbook single files and main index.
I guess there's nothing more to do, isn't it?
By the way, if you want I can move it and left you free for other activities.
I suppose I only need to
- svn move incubator/library incubator/test to /library /test
- promote documentation
- update docbook single files and main index. I guess there's nothing more to do, isn't it?
Sorry, Simone, I didn't realize that it was documented that only a Zender could perform the actual promotion. I have confidence that you would have no problem doing it, but in the interest of following the rules, I'll go ahead and make promote it today as soon as possible.
Thanks again!
Thanks again!
r7247 includes 100% of Technorati API support and 90% of unit tests.
Next step is to reach 99% of unit tests.
It shouldn't be hard, all classes have been stressed by custom tests thus I only need to validate them, no more changes should be required.
Before starting the documentation it would be great to receive an overall feedback about the final product.