This module is designed to solve a simple task.
If you post a comment, and the previous comment (to the same node or comment) was also yours, it looks better, if those comments join together. So that's what the module is supposed to do.
At the moment (6.x-1.0-beta1) it has no almost no options. You just turn it on and get the result. I am using it on my Drupal forum, and I am happy with it.
However, I am ready to hear your requests and ideas!

P.S. Yes, I've read the recomendation to have a few weeks or months of commits, but now the module works on my site and seems perfectly stable. I guess if it goes to public, I will have new ideas and requests, but now I can't invent possible improvements.
Sandbox link: http://drupal.org/sandbox/Sander/1081930

Comments

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Needs review
attiks’s picture

attiks commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, some minor coding style problems (ex. } else { ), but nothing too bad

mlncn’s picture

mlncn commented
Status: Reviewed & tested by the community » Needs work

Comments before functions should be in the form /** ... */ and "Implements hook_xyz()." http://drupal.org/coding-standards

But yes, this is very close to ready to be promoted to full status. Thanks!

lslinnet’s picture

lslinnet commented

Nice little functionality that this module implements, got a few observations:

In the install file your using 

db_query("DELETE FROM {variable} WHERE name = 'comment_join'");

could be replaced with the saver function

variable_del('comment_join');

http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/variab...

Line 41 and 64 in install, your using the constant tiny instead of the string "tiny"

In install the "indexes" are not used so could be removed.

In the .module file

Line 12 to 15 could be optimized to

  $res = "\n\n<strong>" . $info;
  $res .= " ". date("m/d/Y",$time);
  $res .= " - ";
  $res .= date("H:i",$time);
  $res .= "</strong>";

further please make this themable so we can change the looks of it without hacking the module.

There is a lot of inconsistent control structure code style (http://drupal.org/coding-standards#controlstruct)
Line: 25, 34, 41, 49, 54, 69, 80, 85, 107

Line 100: You should use spaces for indentation instead of tabs.

Line 58 and 89 - a semicolon after control structure is a bit confusing.

line 60 and 91 - would be great candidates to be moved to a theme function or template instead.

Another general thing in your coding style is the missing space between params you pass into function calls (http://drupal.org/coding-standards#functcall)
Also quite a few places where your missing space before and after = (http://drupal.org/coding-standards#operators)

__Sander__’s picture

__Sander__ commented

Thank you for all the comments!
I will try to follow your recomendations as soon as possible!

__Sander__’s picture

__Sander__ commented

I fixed everything exept for

further please make this themable so we can change the looks of it without hacking the module.

and

line 60 and 91 - would be great candidates to be moved to a theme function or template instead.

Could you please point me on how to do those things properly?

__Sander__’s picture

__Sander__ commented
Status: Needs work » Active
mlncn’s picture

mlncn commented
Status: Active » Fixed

Congratulations! You know can promote sandbox projects to full status ones.

Nice work and when you would like further review for any module please do ask for one at http://groups.drupal.org/peer-review/requests

Further cleanup issues introduced here and new ones can be filed on your project issue queue. Thank you for your contribution and i look forward to seeing its refinement and development and your continued involvement in Drupal!

benjamin, agaric

__Sander__’s picture

__Sander__ commented

Thank you!

mlncn’s picture

mlncn commented

And as for your questions on using theme functions or templates, see http://drupal.org/node/165706

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.