[D7] Search API Multi Aggregate

Hi Julian Maurice,

A few points, Coder gives a few errors:
search_api_multi_aggregate.module

Line 1: End of line character is invalid; expected "\n" but found "\r\n" [sniffer_generic_files_lineendings_invalideolchar]

severity: minorreview: sniffer_internal_lineendings_mixed
Line 1: File has mixed line endings; this may cause incorrect results [sniffer_internal_lineendings_mixed]

severity: minorreview: sniffer_whitespace_fileend_fileend
Line 26: Files must end in a single new line character [sniffer_whitespace_fileend_fileend]

callback_add_multi_aggregation.inc

Line 1: End of line character is invalid; expected "\n" but found "\r\n" [sniffer_generic_files_lineendings_invalideolchar]

severity: minorreview: sniffer_internal_lineendings_mixed
Line 1: File has mixed line endings; this may cause incorrect results [sniffer_internal_lineendings_mixed]


severity: minorreview: sniffer_whitespace_fileend_fileend
Line 247: Files must end in a single new line character [sniffer_whitespace_fileend_fileend]

With coder set to strict it finds more errors, try to fix these to (see attachment).
I think a lot of these errors are caused because of the line endings on your system (are you on a windows system?). I have had the same problem and used EOL conversion on my notepad++ to remove all errors about the docblocks.

Manual review,
code looks ok. I would add a few inline comments to the functions in callback_add_multi_aggregation.inc for readability.

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 :-)

I can confirm that the modules does not generate any errors or warnings when run through the coder module.

One small suggestion would be to reconsider consider the module name, or at least verify that all of the underscores don't introduce the possibility for namespace collisions.

I assume the concatenation going on in lines 14-16 of the .module are to stay under the 80 character limit. But I'm not sure that limit applies to code. Either way, that's a pretty clunky workaround and we should probably come up with a better solution.

Hi Julian,

Naming
The potential problem with the module name would be, for example, if there were a HOOK_aggregate function. In that case, the function search_api_multi_aggregate() becomes an implementation of HOOK_aggregate by a module named `search_api_multi`. I don't think there is an official position on this, so it shouldn't stop this from being RTBC, but it's worth thinking about. Here is the original article I read that made me think about it: http://drupalize.me/blog/201301/naming-things-hard

Line length
I'm certainly not in a position to override the documentation you referenced. But I would imagine that concatenating strings wasn't the original intention of that standard. A quick spot check reveals that MANY of core modules have lines of code WELL above 80 characters (check out line 50 of the core user.module file: 1,020 characters!) And it's my opinion that line breaking/concatenating actually makes the code less readable. I'd love to hear someone else chime in on this.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1917088

Project 2: http://drupal.org/node/1844646

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message.

Your project page is way more informative than the README.txt file, you should improve that.

Module code looks good.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Still looks good, and it's been over a month.

Thanks for your contribution, Julian Maurice!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.