Support from Acquia helps fund testing for Drupal Acquia logo

Comments

juliangb’s picture

NB: Reviewing these, a lot of these are very minor and easy to fix.

Some of the recommendations that simply refer to comments, I will try to fix directly without a patch.

I'll also run patches through coder before committing from now on, to try to minimise any other warnings that slip in.

Raphael Dürst’s picture

Good point, I will run my patches through coder too from now on.

If you want even stronger coding standards, you can also use Coder Tough Love.

juliangb’s picture

Good point. Let's use Tough Love as well for patches.

It sets the bar higher, but it will be good for us.

dbt102’s picture

Thanks for bringing up this issue. I was unaware of these two modules but will start learning them now. Hopefully, it will let me be more help in getting PM ready for a D7 release. You're doing a great job in moving things along... Keep up the good work!

juliangb’s picture

With CTL (Coder Tough Love) too:

Coder found 11 projects, 141 files, 5 critical warnings, 1466 normal warnings, 716 minor warnings, 0 warnings were flagged to be ignored

I'm going to try to solve these in incremental patches where sections are not being worked on in other issues, rather than massive patches that are impossible to read.

Also attached, some screenshots that demonstrate the settings I'm using to test, in case you want to try as well.

If you set these in the "Settings" tab, then you can run this same review whenever you like by clicking the "Default" tab.

dbt102’s picture

I installed Coder and Coder TL on my PM sandbox OK.

Thanks for the screenshots. They were helpful. I just ran review with Comment #5 settings and get

Coder found 11 projects, 141 files, 5 critical warnings, 1470 normal warnings, 715 minor warnings, 0 warnings were flagged to be ignored

This is close to that reported in #5 . I probably have an patch or two installed at this point which likely accounts for the difference.

Nice to know we have a quantifiable benchmark to gauge progress towards PM D7 release candidate.

juliangb’s picture

Status: Active » Needs review
FileSize
14.88 KB

Here's an initial patch to tackle more coder warnings.

I hope by doing a few batches like this it will be less frustrating for those making real patches!

This one is for the pmtimetracking files.
- The patch file throws 2 minor warnings - these are false positives from the patch file structure
- There will be some complaints about sentence case vs. title case left in the files after applying the patches. I disagreed with these ones.

dbt102’s picture

Patch applied cleanly. Only warning found by coder was "Use sentence case, not title case, for end-user strings."

Coder found 1 projects, 6 files, 10 normal warnings, 0 warnings were flagged to be ignored

PM timetracking module performed ok.

juliangb’s picture

Status: Needs review » Active

Thanks for testing. I've committed this one.

Keeping the issue open to work through the remaining issues.

As an aside, I've opened up the coder functionality on the PM demo site, in case it helps anyone that doesn't have their own set up going.

More details: http://groups.drupal.org/node/296793#comment-922568

Raphael Dürst’s picture

FileSize
18.52 KB

Here's a patch for pmproject.

There are also some complaints about sentence case.

And there is one complaint about the pmproject.js file. But I think we don't use this file anymore. Because it is still loaded in hook_init(), but I don't think, it actually does anything.

Raphael Dürst’s picture

Status: Active » Needs review

And again, needs review.

juliangb’s picture

I don't think we need the js file either, and there may be other js files we can do without - due to using Drupal core's ajax features.

Could you open a new issue to track this, so that we can keep this one for purely style issues?

dbt102’s picture

Patch applied cleanly. Coder said it "found 1 projects, 7 files, 29 normal warnings, 0 warnings were flagged to be ignored".

Then I created new project, saved and edited it OK. Project views looks ok.

Also, as per #10 comment... I deleted the pmproject.js file. This then reduced the coder files and normal warnings count by 1. Went back and tested it again without file, and everting seemed to work OK.

juliangb’s picture

Thanks for this. Very minor...

+++ b/pmproject/pmproject.moduleundefined
@@ -269,10 +284,16 @@ function pmproject_field_extra_fields() {
+/**
+ * Change organization_title in all projects.
+ */
 function pmproject_pmorganization_change($organization_nid, $organization_title) {
   db_update('pmproject')
     ->fields(array('organization_title' => $organization_title))
@@ -280,6 +301,9 @@ function pmproject_pmorganization_change($organization_nid, $organization_title)

@@ -280,6 +301,9 @@ function pmproject_pmorganization_change($organization_nid, $organization_title)
     ->execute();
 }
 
+/**
+ * Change manager_title & assigned_title in all projects.
+ */

Could these refer to the hook that their implementing?

+++ b/pmproject/pmproject.testundefined
@@ -3,8 +3,11 @@
+class PmprojectTestCase extends DrupalWebTestCase {

Could we use PMProject as standard in TitleCase?

In other news, I'm keen that our efforts to document here are not just to "tick a box" with coder, and have been setting up an API site. Have a look at http://api.project-management-module.org and let me know of any comments. It covers Project Management and dependency modules, plus extensions such as PM Vista. I hope this will help us, and also new contributors, and be something that we can link to help in productive issue queue discussions.

Raphael Dürst’s picture

FileSize
18.5 KB

I applied these changes in this patch.

The API documentation is very helpfull. And if we add more documentation to our code, it will get even better.

juliangb’s picture

Status: Needs review » Active

Tests pass: Tick
Coder review of patch passes: Tick
Readthrough: Tick
Functional test: Tick

I've committed this, and pushed up to Drupal.org. Thanks!

Great to hear that you think http://api.project-management-module.org will be useful. Please do give me any comments as well as to anything else that could make developing for Project Management easier or faster.

Latest counts:

Coder found 11 projects, 141 files, 5 critical warnings, 1307 normal warnings, 549 minor warnings, 0 warnings were flagged to be ignored

I'm just going to write up a patch for pmtask now.

juliangb’s picture

Status: Active » Needs review
FileSize
34.22 KB

Here's a patch for Tasks.

Note:
- There are some warnings on the JsGantt implementation. I haven't looked at these as I think we'll be changing that section anyway in not too long. (warnings about mixed case, etc, etc).
- There are some warnings about commented out code. These are sections that need to be looked at, and it isn't the place to be changing them in a patch about code style.
- There is one potential security problem flagged by coder in this section. It isn't an issue as the variable it talks about needing sanitizing is already dealt with elsewhere (the pm icons code).

Raphael Dürst’s picture

Status: Needs review » Needs work

Some minor things:

+/**
+ * Implments hook_help().
+ */
 function pmtask_help($path, $arg) {
   $o = '';
 
   switch ($path) {
     case "admin/help#pmtask":
-      $o = '<p>'. t("Provides task support for Project Management") .'</p>';
+      $o = '<p>' . t("Provides task support for Project Management") . '</p>';
       break;
   }
 
   return $o;
 }
 
+/**
+ * Implments hook_permission().
+ */
 function pmtask_permission() {
   return array(
     'Project Management Task: access' => array(
@@ -62,10 +68,16 @@ function pmtask_permission() {
   );
 }

Implments is missing an e.

And on line 1266, insert a space between // and }.

Besides that it seems fine.

juliangb’s picture

Status: Needs work » Needs review
FileSize
34.51 KB

Thanks for the review.

Amended patch - which fixes those three things, plus an error that I noticed on the node add form that was due to my previous patch.

Raphael Dürst’s picture

Patch applies cleanly, Coder looks fine and readthrough makes sense.

I'm going to make a patch for pmorganization now.

Raphael Dürst’s picture

FileSize
11.72 KB

Ok, here's a patch for pmorganization.

There are a lot of warnings about title case, I ignored them.

And there are also warnings about using mail instead of email, but that's not in the scope of this issue.

dbt102’s picture

Patch applies cleanly.

Coder looks fine and readthrough makes sense to me for the most part. The only thing I would note from Coder is typical to ...

Line 279: Core uses "e-mail" in end-user text and "mail" elsewhere (database, function names, etc.) [coder_tough_love_8]
'#default_value' => isset($node->email) ? $node->email : NULL,

(I note it here because I just don't know what it means, not that I think it really important at this point.)

Functional testing produces no new error that I could find.

juliangb’s picture

Status: Needs review » Active

Thanks for patches and reviews. I've committed #19 and #21.

Re the email / e-mail / mail point: Agree that it isn't something worth changing here. It doesn't add useful documentation like other changes. I guess we'll try to be more diligent on this when moving the fields to field api.

Latest:

Coder found 11 projects, 134 files, 5 critical warnings, 1133 normal warnings, 402 minor warnings, 0 warnings were flagged to be ignored
juliangb’s picture

Status: Active » Needs review
FileSize
4.52 KB

A patch for team. Simple one.

A few leftover warnings relate to commented out code that we still need to handle in the port.

dbt102’s picture

Patch applied cleanly. Went through team add, edit, delete and view and all functioned OK.

Raphael Dürst’s picture

FileSize
20.78 KB

Patch for ticket. Only warnings about title case, which I ignored.

juliangb’s picture

Status: Needs review » Active

I've committed #24 and #26, thanks!

Latest counts:

Coder found 11 projects, 134 files, 5 critical warnings, 1073 normal warnings, 309 minor warnings, 0 warnings were flagged to be ignored
juliangb’s picture

Status: Active » Needs review
FileSize
54.8 KB

Here's a patch for pm.module.

There are a few false positives remaining (including the one critical, and some that I've ignored for now as they involve code changes that I don't want stuck in the middle of a large boring patch).

Status: Needs review » Needs work

The last submitted patch, pm-coder-2.patch, failed testing.

juliangb’s picture

Status: Needs work » Needs review

#28: pm-coder-2.patch queued for re-testing.

dbt102’s picture

Patch applied cleanly. Clicked around for general usage and could find no major issues due to this patch. Worked OK for me.

juliangb’s picture

Status: Needs review » Active

Thanks for testing. I've committed this patch.

Latest counts:

Coder found 11 projects, 134 files, 5 critical warnings, 927 normal warnings, 229 minor warnings, 0 warnings were flagged to be ignored
juliangb’s picture

After a few other issues (each patch has to pass a coder check on the code that is being changed):

Coder found 11 projects, 134 files, 5 critical warnings, 890 normal warnings, 193 minor warnings, 0 warnings were flagged to be ignored

Heading in the right direction, and hope that by embedding these checks into the process this is becoming less of a chore for you all.

juliangb’s picture

Status: Active » Needs review
FileSize
487 bytes
5.78 KB
14.91 KB
8.61 KB

I had another crack at these and started ignoring false positives.

Patches attached, split by module for easier reviewing.

Latest counts with all applied:

Coder found 11 projects, 130 files, 2 critical warnings, 694 normal warnings, 44 minor warnings, 10 warnings were flagged to be ignored
juliangb’s picture

Status: Needs review » Active

Committed those patches.

juliangb’s picture

Issue summary: View changes
FileSize
9.04 KB

Next iteration on this - solves most of the warnings on the PM expense module.

Coder found 11 projects, 130 files, 2 critical warnings, 736 normal warnings, 7 minor warnings, 10 warnings were flagged to be ignored
juliangb’s picture

Status: Active » Needs review
juliangb’s picture

Status: Needs review » Active

Committed latest patch here.

  • Commit cf0c47a on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1984336 by juliangb: Cleanup: Resolve Coder Warnings.
    
  • Commit be23af7 on 7.x-1.x, 7.x-2.x authored by Raphael Dürst, committed by juliangb:
    Issue #1984336 by Raphael Durst, juliangb: Cleanup: Resolve Coder...
  • Commit c83a1b3 on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1984336 by juliangb, Raphael Drst: Cleanup: Resolve Coder...
  • Commit de87d8b on 7.x-1.x, 7.x-2.x authored by Raphael Dürst, committed by juliangb:
    Issue #1984336 by juliangb, Raphael Drst: Cleanup: Resolve Coder...
  • Commit b84da0e on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1984336 by juliangb, Raphael Drst: Cleanup: Resolve Coder...
  • Commit 3a47e98 on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1984336 by juliangb, Raphael Drst: Cleanup: Resolve Coder...
  • Commit 227d621 on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1984336 by juliangb, Raphael Drst: Cleanup: Resolve Coder...
  • Commit 71c5061 on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1984336 by juliangb, Raphael Drst: Cleanup: Resolve Coder...
  • Commit 263cf0d on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1984336 by juliangb, Raphael Drst: Edit Issue Cleanup: Resolve...
juliangb’s picture

Status: Active » Fixed

We should now judge code style using Pareview - see #2215477: Cleanup: Resolve pareview.sh warnings.

Status: Fixed » Closed (fixed)

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