[D7] Views Fast Forward

Strange, these issues where not pointed out by using the recommended Coder (Review) module. Yet, most of them are fixed now and changes are pushed.

The Pareview checker now still complains about the casing of class and method names, yet CamelCasing Views class names and lowerCasing method names (as it suggests) breaks Views, so my bet is that these errors can be ignored?

If I am missing something...let me know!

Thanks for pointing that out and for the explanation Sajiniantony.
I adjusted the code and pushed the changes.

Hi Yuriy

Thank you for the review!

1. The problem with Pareview is that you used an incorrect link to the git repo. "timusan" should be "Timusan", with a capital T. The Drupal code hosting is case sensitive. The correct link is as posted in my issue description:

http://git.drupal.org/sandbox/Timusan/2422313.git

However, Pareview seems to ignore this fact and lower-case the URL when trying to hotlink to the rendered review it produces. This seems to be a bug in Pareview itself. You will thus have to manually copy and past the given git repo link into the Pareview homepage for it to work.

The default branch is 7.x-1.x as stated on the sandbox version control tab.

2. Which README format is preferred? Mardown or plain text?

3. I rather shorten the description for this information is already provided via hook_help(). Yet, I see many modules with even longer descriptions. Do you consider this a possible stopper?

4. The hook_help() code is directly derived from the recommended code on drupal.org itself: https://www.drupal.org/node/161085#hook_help . This documentation should be considered wrong?

In the meantime, contrary to sajiniantony's comments on using st() instead of t() during installation, I reverted back to use only get_t() inside the hook_install() instead, as pointed out by the automated testing system / documentation. This change is now committed and pushed.

@naveenvalecha: Done!

Thank you, Klausi, for the detailed review and for your note on reviewing "needs review" statuses only (apologies for that).

Regarding your manual review notes:

1. you should only have one README file: either delete README.txt or README.md.

I still had this pending on which was the best format, I have now opted for the most "rich" one and removed the plain text version.

2. "define('VIEWS_FAST_FORWARD_PATH', drupal_get_path('module', 'views_fast_forward'));": that path lookup will run on every single page request even if your module is not involved at all - is this really necessary just fro the Views API hook?

I had done this for better readability throughout the code, yet this is indeed a case where performance outweighs readability, I have removed this constant and inlined it instead.

3. options_definition_alter(): is this method really needed? Does the same as options_definition(), why? Please add a comment or remove.

option_definition_alter() seems to be quite redundant indeed. I removed it in favor of the standard option_definition() method.

4. Ah, the module only works if I use a View that uses the fields style? Please add that to the README.

Correct, for now I have opted to support only "fields" style Views. This indeed had to be clearly mentioned on the sandbox page and in the README. Sorry for that, it has now been added to both.

All the changes have been committed in separate, descriptive commits and pushed onto the 7.x-1.x branch.

I will review three new projects as soon as possible.

Cheers,
Tim

I reviewed three other modules which had status "Needs review" (and some I changed to "Needs work" after I reviewed). The new reviews are number 4, 5 and 6 in the issue description.

Cheers,
Tim

Hi Klausi

Thank you too for your review time on this project and for upgrading my account.
I will take the time to read through the recommended texts and try to hangout on IRC more often.

Thanks to all other review folks as well, I appreciate you taking time to review my code.

Cheers,
Tim