[D7] race_time

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxrobbertv2158841git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi,

You code is still on master branch, move it to 7.x-1.x. Read more at https://drupal.org/empty-git-master

format.js - has lines which is not terminated by ; (semicolon). Please correct formatting issues as per JavaScript coding standards. https://drupal.org/node/172169

format.js - Review the inline comments as per Doxygen formatting conventions. Exceeds 80 chars (http://drupal.org/node/1354)

Go to your module page >> Edit >> Default branch and set 7.x as your default branch. Then you will be able to delete the master branch by $git push origin :master

Hi

Thanks for your contribution, I've reviewed your module and have found the following minor issues:

1. racetime_validate($element)
function doesn't actually do anything?

2. racetime.module line 362
missed the d of formatted

As a note, I found it a bit confusing that the field type added is called Time, perhaps it should be called Race Time, as per the module name.

Also I was expecting it to appear as a widget for Date, rather than it's own field type..

Thank you

Michel

Hi robbertv,

I agree with michel_v that the field type should be called as the module name. I also found the following issues when reviewing your module:

• In racetime_field_widget_form(): $widget['#description'] currently reads "Add description here". Please change it to a useful message that instructs users the correct format the input should have, like the number of decimals expected. Could be something like: "Enter time in 00:00.00 format" or "123456 will be interpreted as 12:34.56".
• When playing with the jquery widget I experienced some unexpected behavior. I have configured a field with 2 decimals, and say I want a value of 1 second. The following happens:
  • input => parsed
  • 1 or 1. => .1
  • 1.0 => .10
  • 1.00 => 10:1.00

  I know I can just type "000100" to be parsed as "00:01.00", but it just doesn't feel as natural, and it would be a great UI improvement if you could take this into account.

Cheers!

I tested your module and it works as expected.

hi,
i read the code and here are ny notes :

- in hook_field_schema
'time' => array( can we please name the column 'racetime' as the providing module name ?

- in the .module file
1) racetime_field_widget_form(& $form, &$form_state, $field, $instance, $langcode, $items, $delta, $element)
there shouldn't be a space between & and $form.

2) isset ($items[$delta]['time']) should be isset($items[$delta]['time']) (no space after the function name)

-in _racetime_add_js
Same space issues :
1) & drupal_static(__FUNCTION__); => &drupal_static(__FUNCTION__);
2) !empty ($racetime_js_added) => !empty($racetime_js_added)
Actually, i'm not sure what _racetime_add_js() is for. as it returns nothing, i don't see how the drupal_static will be used. i guess the need is to add the js file only once in the page call, this is already the case with drupal_add_js for files, it adds them once.

-about the js file
We need to have a Js Drupal behavior for this : https://drupal.org/node/304258#drupal-behaviors

Thanks

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found blocking problems with the project) or "reviewed & tested by the community" (you found no major flaws). Could you go back to those and set the status accordingly?

Review of the 7.x-1.x branch (commit e41333c):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. "@returns {string}" should be "@return string", see https://www.drupal.org/coding-standards/docs#return
2. racetime_field_formatter_settings_summary(): shouldn't this show an example format how the value will be displayed?
3. _racetime_add_js() is unused and can be removed?
4. theme_racetime(): the implode() is just complicating the code here, why don't you just put the class directly into $html?

But otherwise looks good to me, setting to RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

Oops, forgot attachment.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, few below issues.

Review of the 7.x-1.x branch (commit c2238ab):

FILE: /var/www/drupal-7-pareview/pareview_temp/format.js
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
13 | ERROR | Doc comment short description must be on a single line, further
| | text should be a separate paragraph
80 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
--------------------------------------------------------------------------------

Manual Review

Project Page: Extend this little bit more, better to go through Tips for a great project page again.

(+) racetime.module: <?PHP ALLCAPS is a bad taste instead it should be <?php.

I tested this module functionality and found for invalid time it shows Invalid Time alert message but don't prevent the form submission. It should stop form submission with appropriate message for better user experience.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Inline comments and docs looks good to me so...

Thanks for your contribution, robbertv!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.