ApacheSolr Numeric Facets

For what it's worth, consider me a +1 on including this as a new module. I have some plans to contribute to this and potentially use it as well.

• broken line endings in README.txt ('\r'). Use unix line endings ('\n')
• same for the info file
• lines in README.txt should not exceed 80 characters.
• Remove all old CVS $Id tags from all files, not needed anymore.
• apachesolr_numeric_facets_block(): why are there 2 "break;" statements at the end of the view case? The last one should be enough.
• "//loop through our results once to find out the min and max price": comments should start capitalised and end with a ".". There should be a space after "//".
• apachesolr_numeric_facets_admin_settings(): don't add $form['no-facets-message'] at all if there are field instances available. Use an if statement instead of the #access work around.

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

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project doc umentation.
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  apachesolr_numeric_facets.install:19:  //Remove modules variables
  apachesolr_numeric_facets.module:207:    //parse the division input from our block textarea field
  apachesolr_numeric_facets.module:215:      //if the range is -inf, use the $neg_inf variable defined above
  apachesolr_numeric_facets.module:222:      //if the range is inf, use the $pos_inf variable defined above
  apachesolr_numeric_facets.module:259:      //The sorting logic is determined by the checkbox in the block configuration
  apachesolr_numeric_facets.module:272:      //Set the display text.
  apachesolr_numeric_facets.module:296:        //Pass all of the count, text, etc. information back through the $link variable
  

• ./apachesolr_numeric_facets.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

        //The sorting logic is determined by the checkbox in the block configuration
          //Pass all of the count, text, etc. information back through the $link variable
  

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

  ./apachesolr_numeric_facets.info:      ASCII text, with CRLF line terminators
  apachesolr_numeric_facets.info
  apachesolr_numeric_facets.install
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./apachesolr_numeric_facets.module ./apachesolr_numeric_facets.install ./README.txt ./apachesolr_numeric_facets.admin.inc ./apachesolr                                                                          _numeric_facets.info
  

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

Hi!

I have attached the latest review script results for you. Then, some additions:

Git

There are already tags in your repo. You will really not need them before you have turned it into a full project and start releasing. Also there are still files in your master branch. I recommend you take some time with http://drupal.org/node/1014932, especially http://drupal.org/node/1127732 (step 5).

(I was just about opening you demo site for a better understanding of your modules intent, but it was actually not reachable - don't know why. Blank page.)

Manual review:

• apachesolr_numeric_facets.module -- Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
• apachesolr_numeric_facets.admin.inc -- ln 25, inconsistent indentation; +2 on '#default_value'
• apachesolr_numeric_facets_menu -- 1 space between key/val pairs; see http://drupal.org/node/318#array
• apachesolr_numeric_facets_block -- add a break; before each case.
• apachesolr_numeric_facets_block -- case 'configure', indent 2 spaces on $form['apachesolr_numeric_facets_sort_by_hits']

Guys, come on. We are holding up a legitimate project application for 4 months over what is now a few spaces and line endings. I'm all for code review, both manual and automated, but we're not talking about security issues with the code at this point.

Can we please just approve his application for a git account and promote it to a project to allow for more collaborators to assist? I have been personally contacted by a number of developers looking for this very module (since I created a similar custom version of this) and I fear we're all starting to fork off similar modules and duplicate work.

I have my git account, if you want I'll create it and you can add brycesenz as a co-maintainer - whatever. I just want it up so people can start using it and contributing bugfixes, enhancements, etc. Thank you.

I'm probably not the best person to handle security review or deeper testing with Solr (not having worked with it before). Just trying to help out with the low-level checks here...is there a more experienced reviewer out there that could help out?

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:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

• apachesolr_numeric_facets_menu(): 'access argument' is missing.
• 'Use this screen to select which numeric/computed fields should be handled using Apache Solr Custom Fields': does not match the current module name?

I know, getting feedback on your application took long, you can speed up the process with #1410826: [META] Review bonus.

Good module, good work!

Please, correct some minor issues about following coding guidelines . I attached the automated report.

I know, it could be a tedious and meaningless work at first glance, but these modifications will provide an absolute consistent look and feel of your code to everybody. See: http://drupal.org/coding-standards

If these changes will be done, I could not see any other problem.

Don't switch to needs work if there are only some minor/few issues thrown by automated tools

It has been 8 months of back-and-forth over minor coding issues. Can we please please just approve the application so Bryce can contribute this back to the community?

I have an existing d.org git account - would you all feel more comfortable about this if I just promoted it to a project and changed the application request to add bryce as a co-maintainer?

Please see the project application workflow: http://drupal.org/node/532400
Only applications that are in the "reviewed & tested by the community" state will get approved. Please review and test the module, then set it to RTBC if you think it is ready.

Sorry for the delay, but there are no reviews of other project applications listed in the issue summary as strongly recommended in the application documentation.

OK, will do then. I have tested with the module posted in the sandbox and also have my own that is ~95% similar to it. I'd like to potentially combine the two approaches after this is promoted as a project.

@brycesenz: the coding standards are not changing that much, but the automated review tools are getting better and better at detecting coding standard errors.

Sorry that the process feels a bit painful for you. Don't get me wrong - I don't want to discourage contributions, but we also need to do some basic quality assurance on drupal.org. Projects should be secure, they should not violate any licenses, they should not duplicate existing modules. And of course we don't want spammers to squat our project name space.

So while there is more automation planned for the review process we need to help each other right now to keep it going.

Now a review:

1. 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
2. The errors detected by automated tools listed in #19 are still not fixed.
3. "'info' => t($info),": do not pass dynamic variables to t() where possible because then the string cannot be detected by translation extraction tools. Also elsewhere.
4. apachesolr_numeric_facets_admin_settings(): doc block is wrong, this is not a hook. See http://drupal.org/node/1354#forms

You should definitively fix those issues, but they are no blockers.

Thanks for your contribution, brycesenz! 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.

Thanks to the dedicated reviewer(s) as well.