UC Tracking Links

Automated review
Please keep in mind that this is primarily a high level check that does not replace but eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.

Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):

FILE: ...ites\default\modules\reviews\uc_tracking_links\uc_tracking_links.module
--------------------------------------------------------------------------------
FOUND 38 ERROR(S) AND 3 WARNING(S) AFFECTING 17 LINE(S)
--------------------------------------------------------------------------------
   1 | ERROR   | End of line character is invalid; expected "\n" but found
     |         | "\r\n"
   2 | ERROR   | Missing file doc comment
   3 | ERROR   | Missing function doc comment
   5 | WARNING | Line exceeds 80 characters; contains 100 characters
   5 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
   9 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  26 | ERROR   | Line indented incorrectly; expected at least 4 spaces, found 2
  26 | ERROR   | Inline comments must start with a capital letter
  26 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  30 | ERROR   | Line indented incorrectly; expected at least 4 spaces, found 2
  30 | ERROR   | Inline comments must start with a capital letter
  30 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  34 | ERROR   | Line indented incorrectly; expected at least 6 spaces, found 4
  34 | ERROR   | Inline comments must start with a capital letter
  34 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  39 | ERROR   | Line indented incorrectly; expected at least 8 spaces, found 4
  39 | ERROR   | Inline comments must start with a capital letter
  39 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  47 | ERROR   | Line indented incorrectly; expected at least 4 spaces, found 2
  47 | ERROR   | Inline comments must start with a capital letter
  47 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  65 | ERROR   | Line indented incorrectly; expected at least 6 spaces, found 4
  65 | ERROR   | Inline comments must start with a capital letter
  65 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  78 | ERROR   | Line indented incorrectly; expected at least 6 spaces, found 4
  78 | ERROR   | Inline comments must start with a capital letter
  78 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  93 | ERROR   | Line indented incorrectly; expected at least 12 spaces, found
     |         | 10
  93 | ERROR   | Inline comments must start with a capital letter
  93 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
  95 | WARNING | Line exceeds 80 characters; contains 87 characters
  95 | ERROR   | Line indented incorrectly; expected at least 14 spaces, found
     |         | 12
  95 | ERROR   | Inline comments must start with a capital letter
  95 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 100 | WARNING | Line exceeds 80 characters; contains 113 characters
 100 | ERROR   | Line indented incorrectly; expected at least 14 spaces, found
     |         | 12
 100 | ERROR   | Inline comments must start with a capital letter
 105 | ERROR   | Line indented incorrectly; expected at least 12 spaces, found
     |         | 10
 105 | ERROR   | Inline comments must start with a capital letter
 105 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
     |         | question marks
 120 | ERROR   | Break statement indented incorrectly; expected 6 spaces, found
     |         | 4
--------------------------------------------------------------------------------

Manual Review

• Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.
• I'm not a fan of your style of commenting sections, in relation to lines 17,18 and 19 of uc_tracking_links.module; if you document functions clearly this is not required.

Code Reviews
When running the Coder module make sure that it is set to 'minor' and always try to use the latest development versions for up to date checks. There are also a host of optional add ons that run with Coder and help to spot errors in your code such as Coder Tough Love, the Translation Template Extractor and Text Review to name some of the more popular ones. There is also a really good script called PAReview.sh that does a great job of spotting errors, its still a sandbox project though so is liable to changes. Drupal Code Sniffer is also great at catching all sorts of errors within your code but can be a bit tricky to set up.

RE:

PAReview.sh require a bash, whilst I'm on a Windows box

You can still get a Linux shell running on Windows. For git access I use msysgit which has a GUI and a shell environment. Personally I only use the shell environment.

It allows you full access to many 'Linux Tools' and the ability to run bash scripts, such as PAReview.sh.

Havent had a chance to check your code again, will try and revisit this later.

btw: you can also use http://ventral.org/pareview - it is an online service providing pareview.sh tests

Could you explain how your module provides something that uc_tracking does not?

It seems that Patrickd's online reviewer actually has some issues with your code I am sorry to say. Dont worry though they are fairly minor points though.

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...sites/all/modules/pareview_temp/test_candidate/uc_tracking_links.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AND 1 WARNING(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
     2 | ERROR   | You must use "/**" style comments for a file comment
    16 | WARNING | Line exceeds 80 characters; contains 81 characters
   139 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
       |         | question marks
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards).
  

  
  FILE: ...sites/all/modules/pareview_temp/test_candidate/uc_tracking_links.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   40 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
      |       | question marks
  --------------------------------------------------------------------------------
  

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• Please use real headings on your project page.
• Please provide more useful Git commit messages. "Internal development" does not say anything. See also http://drupal.org//node/52287
• uc_tracking_links_get_list_of_carriers(): I think you should provide your own hook, so that other modules can add additional carriers. module_invoke_all() is used to invoke hooks in other modules. And a uc_tracking_links.api.php would be nice if you provide that hook.
• $arg1 is really a bad variable name. Use more self-explaining names as $order, $shipment etc.
• uc_tracking_links_get_list_of_carriers(): strings passed to t() should be in English.
• This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project if you wish.

Hi,

You still have some small coding standards issues:

uc_tracking_links.module
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
20 | ERROR | Array indentation error, expected 4 spaces but found 6
20 | ERROR | Array closing indentation error, expected 4 spaces but found 6
25 | ERROR | Array indentation error, expected 4 spaces but found 6
25 | ERROR | Array closing indentation error, expected 4 spaces but found 6
30 | ERROR | Array indentation error, expected 4 spaces but found 6
30 | ERROR | Array closing indentation error, expected 4 spaces but found 6
34 | ERROR | Array indentation error, expected 4 spaces but found 6
34 | ERROR | Array closing indentation error, expected 4 spaces but found 6
39 | ERROR | Array indentation error, expected 4 spaces but found 6
39 | ERROR | Array closing indentation error, expected 4 spaces but found 6
44 | ERROR | Array indentation error, expected 4 spaces but found 6
44 | ERROR | Array closing indentation error, expected 4 spaces but found 6
--------------------------------------------------------------------------------

Please do not switch issues to "needs work" without giving a manual review.

Get a review bonus and we will come back to your application sooner.

I am only interesting in helping out, not to get any bonus, but I have missed the part that you should not change status after a automatic review, thought I read all the documentation about reviews, but must have missed that, sorry...

PAReview.sh doesn't show any errors.
I have done a manual review and it looks great. Maybe if you are open to any suggestions, you can extend the module to add the trackers and links through module administration, rather than harcoding / sub-modules.

I think this should be marked 'reviewed and tested by the community'. Anybody disagree? Another way to handle trackers could be added later on.

@f4k1r & @MiSc: don't hesitate to set the issue to RTBC if you think it is ready.

Thanks for your contribution, mandreato! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.