Hi,

I did review this module with the coder module and coder tough love and did patch some minor things.
Hope that helps this awesome module a bit :)

Added description to administrative menu items.
Some tabs and spaces at lines end.
Added @file to the views and install file.

t() should not be used for menu's title and description.

-    'title' => t('NAT'),
-    'description' => t('Establish node - node relationships via the taxonomy module.'),
+    'title' => 'NAT',
+    'description' => 'Establish node - node relationships via the taxonomy module.',

Installation should use st() instead of t().

-  drupal_set_message(t('NAT module: Installation script complete.'));
+  drupal_set_message(st('NAT module: Installation script complete.'));

Installation should use update_sql instead of db_query

-  $result = db_query('SELECT n.nid, n.tid, td.vid FROM {nat} n INNER JOIN {term_data} td USING (tid)');
+  $result = update_sql("SELECT n.nid, n.tid, td.vid FROM {nat} n INNER JOIN {term_data} td USING (tid)");

I still let this query use db_query because I believe you intetionally wanted to hide a load of queries but still changed teh query to use double quotes.
<?
- db_query('UPDATE {nat} SET vid = %d WHERE nid = %d AND tid = %d', $node['vid'], $node['nid'], $node['tid']);
+ db_query("UPDATE {nat} SET vid = %d WHERE nid = %d AND tid = %d", $node['vid'], $node['nid'], $node['tid']);
?>

CommentFileSizeAuthor
#9 nat-703712-coder-review.patch2.7 KBanantagati
#7 nat-703712-coder-review.patch2.7 KBanantagati
#6 NAT_coder_review_v03.patch2.04 KBkars-t
#2 NAT_coder_review_v02.patch2.07 KBkars-t
#1 NAT_coder_review_v01.patch3.72 KBkars-t

Comments

kars-t’s picture

kars-t commented
StatusFileSize
new NAT_coder_review_v01.patch3.72 KB

Missed the patch as it seems. And I figured that I worked against the latest release and not the def version. Uploading the old patch now but making a new one probably sunday against the HEAD version -_-

kars-t’s picture

kars-t commented
Version: 6.x-1.1-beta3 » 6.x-1.x-dev
StatusFileSize
new NAT_coder_review_v02.patch2.07 KB

New patch against latest 6--1 branch. Less work because most of the things were already fixed.

anantagati’s picture

anantagati commented
Status: Needs review » Needs work

Hi Kars-T, I tried to applied patch but it write error:

patching file nat/includes/nat.views.inc
patch: **** malformed patch at line 20: \ No newline at end of file
kars-t’s picture

kars-t commented

Hi,

great you are back again! I feared NAT was left unmaintained :)

"\ No newline at end of file" shouldn't be a problem.

How did you apply the patch?
IDE or Shell?

anantagati’s picture

anantagati commented

great you are back again! I feared NAT was left unmaintained :)

Thanks. As I am only co-maintainer for Views part of the module I wrote today to maintainer if he will not mind that I will take maintaining of this nice module.

I applied it over shell.

kars-t’s picture

kars-t commented
Status: Needs work » Needs review
StatusFileSize
new NAT_coder_review_v03.patch2.04 KB

Okay used patch -p0 < and got the error as well.

Than I just removed the line and it works. Resubmitting hacked patch.

anantagati’s picture

anantagati commented
StatusFileSize
new nat-703712-coder-review.patch2.7 KB

Patch looks good. Changed one db_query() to update_sql() and removed some trailing spaces to pass coder with minor warning level set.

kars-t’s picture

kars-t commented
Status: Needs review » Reviewed & tested by the community

There were trailing spaces? Gnah! -_-

+++ nat.install	16 Feb 2010 16:34:21 -0000
@@ -55,11 +60,11 @@ function nat_update_2() {
   while ($node = db_fetch_array($result)) {
-    db_query('UPDATE {nat} SET vid = %d WHERE nid = %d AND tid = %d', $node['vid'], $node['nid'], $node['tid']);
+    update_sql(("UPDATE {nat} SET vid = %d WHERE nid = %d AND tid = %d", $node['vid'], $node['nid'], $node['tid']);
   }

I thing this is intenionally db_query because this might give us a lot of update_sql notifcations?

Otherwise its RTBC.

Powered by Dreditor.

anantagati’s picture

anantagati commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new nat-703712-coder-review.patch2.7 KB

You are right. It is better to put db_query().

anantagati’s picture

anantagati commented
Status: Needs review » Fixed

Thank you. Committed.

Status: Fixed » Closed (fixed)

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