[D7] IME

Hi
please check your code via http://pareview.sh/
see: http://pareview.sh/pareview/httpgitdrupalorgsandboxsuhelrangnekar2264685git

you git command is not workable for any one unless you.

You cannot use a command in the format username@git.drupal.org:sandbox/username/repo

It must be in the format http://git.drupal.org/sandbox/username/repo.

and you are working in dev branch you have to move it in a default branch.

It appears you are working in the "7.x-1.0-dev" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.0-dev branch:

manual review

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxsuhelrangnekar2264685git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

No: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

No: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) Major finding

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.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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.

Please note that you've got the name of the main development branch wrong.

You've named it "7.x-1.0-dev". The correct branch name for a main development branch is "7.x-1.x" (Note: "1.x" instead if "1.0" and no "-dev"). Please change the name of the main development branch to follow main branch naming conventions.

Hi Ashwini Kumar,
I have installed your module in a drupal site for testing and I think its working great. The installation instructions are clear enough to get it started.

Manual review,
The code looks fine to me, But, you have written the libraries_load_files() in the hook_init() function. The library is getting loaded on every non-cached page requests and even on homepage. I don't think, the library files are required to load on the homepage. Can you correct this part?

Automated review,
There are minor coding standard issues as per pareview, which can be fixed.

Moving this to "Needs Work" since the library loading on all pages is a serious performance issue. Please explain your reasoning.

@th_tushar and @laceysanderson
Thanks for review.

I have added validation to load IME libraries on Specific pages only.
Pages can be managed from jQuery.ime administration setting pages.

Please review again and Let me know your thoughts.

1) Please reduce clyomatic complexity (if, else) in the function
2) $patterns looks an array to me in the function ime_paths_patterns($ime_enable) when we check it prior to return, but it is initialized as empty string please maintain consistent data type for your variables.

Here's is an updated code which you can probably refer:

function ime_paths_patterns($ime_enable) {
  $patterns = '';
  if (isset($ime_enable)) {
    $pages = variable_get('jquery_ime_pages');
    if (isset($pages) && !empty($pages)) {
      foreach (explode("\n", $pages) as $line) {
        $line = trim($line);
        if (!empty($line)) {
          $patterns[] = $line;
        }
      }
    }
  }
  return $patterns;
}

to:

function ime_paths_patterns($ime_enable) {
  $patterns = array();
  $pages = variable_get('jquery_ime_pages');
  if (isset($ime_enable) && !empty($pages)) {
     foreach (explode("\n", $pages) as $line) {
       $line = trim($line);
       if (!empty($line)) {
         $patterns[] = $line;
       }
     }
  }

  return $patterns;
}

3) Change comments for the function ime_paths_patterns($ime_enable) include @param $ime_enable in the comment and also define its return type in the comment

@pgautam
Refactored function ime_paths_patterns($ime_enable)
Please review again.

Looks good.

Reviews made by other people are not taken into account. The purpose of the whole this process is to show user experience, and the knowledge of Drupal API and common programming skills. It's not the contest or the race.

I'm going to remove the bonus tag, as well as links on reviews from the issue summary.

P.S.: @Sharique you should post a report of your review when you changing the issue status.

I' installed this module and it works fine for me.

Issue reported by @pgautam is fixed now. jQuery.ime js loading on every page is also fixed.

OK. I didn't change the status, just wanted to be sure.

@pingwin4eg @Sharique
Apology for ambiguity I am the maintainer and owner of this module. Therefore I have again added the PAReview: review bonus tag and review links into issue summary.
@ashwinikumar is a mainter of this module as he is busy with other work. So, I am taking care of the review process of respective module.
Please take this points into account and please correct me if I am missing something.

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

• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
  

  ./ime.info:         ASCII text, with CRLF line terminators
  ./js/ime_script.js: ASCII text, with CRLF line terminators
  ./ime.module:       PHP script, ASCII text, with very long lines, with CRLF line terminators
  ime.info
  ime.module
  js/ime_script.js
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /home/klausi/pareview_temp/ime.module
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
  --------------------------------------------------------------------------------
    23 | WARNING | Do not use drupal_add_js() in hook_init(), move it to your
       |         | page/form callback or use hook_page_build() instead
    28 | WARNING | Do not use drupal_add_js() in hook_init(), move it to your
       |         | page/form callback or use hook_page_build() instead
   109 | WARNING | All variables defined by your module must be prefixed with
       |         | your module's name to avoid name collisions with others.
       |         | Expected start with "ime" but found "jquery_ime_enable"
   116 | WARNING | All variables defined by your module must be prefixed with
       |         | your module's name to avoid name collisions with others.
       |         | Expected start with "ime" but found "jquery_ime_ids"
   124 | WARNING | All variables defined by your module must be prefixed with
       |         | your module's name to avoid name collisions with others.
       |         | Expected start with "ime" but found "jquery_ime_pages"
  --------------------------------------------------------------------------------
  

• 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. what is this project useful for? Why and when do people need it? What is the use case? Please improve your project page.
2. ime_admin_form(): doc block is wrong, this is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms on how to document for building functions.
3. ime_help(): all user facing text must run through t() for translation.

So the variable naming is a blocker right now, and please also fix the other issues. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hi klausi
I have made all changes you mention.
Also corrected errors generated with PAReview.sh.
Added use case line on project page.
And resolved the variable naming blocker issue.

Please review again.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

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

• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /home/klausi/pareview_temp/ime.module
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  ---------------------------------------------------------------------------
   166 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
       |         | #attached on the $page render array instead
   171 | WARNING | Do not use drupal_add_js() in hook_page_build(), use
       |         | #attached on the $page render array instead
  ---------------------------------------------------------------------------
  

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. ime_admin_form(): doc block is wrong, this is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms on how to document for building functions.
2. ime_drush_download(): why do you need func_get_args() here? You could just declare an optional function parameter as $path = NULL? Same in drush_ime_post_pm_enable().

But otherwise looks RTBC to me.

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

The docblock change is better, but it should really say something like 'Form constructor for IME settings.' See https://www.drupal.org/coding-standards/docs#forms

(+) The behavior should use the settings that got passed in, and not the global ones. I also suspect the selector should also use the context and you may need a .once() to make this AJAX friendly.

Yay drush command for getting the library! You could also copy down the file directly instead of cloning.

Avoid splitting translatable strings across lines. It makes them harder to translate. Eg, ime_requirements().

You could put the settings form in its own file. This can help with the global memory footprint. See the hook_menu() docs for how to do this.

Can you add the jQUery Update version to the hook_requirements?

You have a few variable_get() calls w/o a default return value. Eg, ime_page_build().

#attached in the render array is preferred over drupal_add_js() and libraries_load_files(). hook_page_build also has an argument, the page by reference, so you can do this.

None of these are blockers.

@ashwinikumar made the application.

@suhel.rangnekar has made most of the recent comments, and the majority of the commits.

Only one of you can get vetted access. I assume it should be @suhel.rangnekar?

Yes @mpdonadio right, I am the owner of the Project and ashwinikumar is an maintainer.
So Please give full project access to suhel.rangnekar

suhel.rangnekar(Owner and Maintainer)
ashwinikumar(Maintainer)

Thanks for your contribution, suhel.rangnekar!

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.

Thanks for the review(s).

IME has now an official release!
for further information see the project page at https://www.drupal.org/project/ime