Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Code currently gives:
Coder found 11 projects, 141 files, 5 critical warnings, 757 normal warnings, 716 minor warnings, 0 warnings were flagged to be ignored
I hope to solve these before we get to release.
Comment | File | Size | Author |
---|---|---|---|
#36 | pm-expense-coder.patch | 9.04 KB | juliangb |
#5 | Coder-Review-Settings_Reviews-Fieldset.png | 71.34 KB | juliangb |
#5 | Coder-Review-Settings_WhatToReview-Fieldset.png | 12.87 KB | juliangb |
#5 | Coder-Review-Settings_PM-Modules.png | 17.55 KB | juliangb |
Comments
Comment #1
juliangb CreditAttribution: juliangb commentedNB: 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.
Comment #2
Raphael Dürst CreditAttribution: Raphael Dürst commentedGood 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.
Comment #3
juliangb CreditAttribution: juliangb commentedGood point. Let's use Tough Love as well for patches.
It sets the bar higher, but it will be good for us.
Comment #4
dbt102 CreditAttribution: dbt102 commentedThanks 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!
Comment #5
juliangb CreditAttribution: juliangb commentedWith CTL (Coder Tough Love) too:
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.
Comment #6
dbt102 CreditAttribution: dbt102 commentedI 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.
Comment #7
juliangb CreditAttribution: juliangb commentedHere'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.
Comment #8
dbt102 CreditAttribution: dbt102 commentedPatch 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.
Comment #9
juliangb CreditAttribution: juliangb commentedThanks 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
Comment #10
Raphael Dürst CreditAttribution: Raphael Dürst commentedHere'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.
Comment #11
Raphael Dürst CreditAttribution: Raphael Dürst commentedAnd again, needs review.
Comment #12
juliangb CreditAttribution: juliangb commentedI 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?
Comment #13
dbt102 CreditAttribution: dbt102 commentedPatch 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.
Comment #14
juliangb CreditAttribution: juliangb commentedThanks for this. Very minor...
Could these refer to the hook that their implementing?
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.
Comment #15
Raphael Dürst CreditAttribution: Raphael Dürst commentedI 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.
Comment #16
juliangb CreditAttribution: juliangb commentedTests 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:
I'm just going to write up a patch for pmtask now.
Comment #17
juliangb CreditAttribution: juliangb commentedHere'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).
Comment #18
Raphael Dürst CreditAttribution: Raphael Dürst commentedSome minor things:
Implments is missing an e.
And on line 1266, insert a space between // and }.
Besides that it seems fine.
Comment #19
juliangb CreditAttribution: juliangb commentedThanks 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.
Comment #20
Raphael Dürst CreditAttribution: Raphael Dürst commentedPatch applies cleanly, Coder looks fine and readthrough makes sense.
I'm going to make a patch for pmorganization now.
Comment #21
Raphael Dürst CreditAttribution: Raphael Dürst commentedOk, 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.
Comment #22
dbt102 CreditAttribution: dbt102 commentedPatch 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.
Comment #23
juliangb CreditAttribution: juliangb commentedThanks 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:
Comment #24
juliangb CreditAttribution: juliangb commentedA patch for team. Simple one.
A few leftover warnings relate to commented out code that we still need to handle in the port.
Comment #25
dbt102 CreditAttribution: dbt102 commentedPatch applied cleanly. Went through team add, edit, delete and view and all functioned OK.
Comment #26
Raphael Dürst CreditAttribution: Raphael Dürst commentedPatch for ticket. Only warnings about title case, which I ignored.
Comment #27
juliangb CreditAttribution: juliangb commentedI've committed #24 and #26, thanks!
Latest counts:
Comment #28
juliangb CreditAttribution: juliangb commentedHere'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).
Comment #30
juliangb CreditAttribution: juliangb commented#28: pm-coder-2.patch queued for re-testing.
Comment #31
dbt102 CreditAttribution: dbt102 commentedPatch applied cleanly. Clicked around for general usage and could find no major issues due to this patch. Worked OK for me.
Comment #32
juliangb CreditAttribution: juliangb commentedThanks for testing. I've committed this patch.
Latest counts:
Comment #33
juliangb CreditAttribution: juliangb commentedAfter a few other issues (each patch has to pass a coder check on the code that is being changed):
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.
Comment #34
juliangb CreditAttribution: juliangb commentedI had another crack at these and started ignoring false positives.
Patches attached, split by module for easier reviewing.
Latest counts with all applied:
Comment #35
juliangb CreditAttribution: juliangb commentedCommitted those patches.
Comment #36
juliangb CreditAttribution: juliangb commentedNext iteration on this - solves most of the warnings on the PM expense module.
Comment #37
juliangb CreditAttribution: juliangb commentedComment #38
juliangb CreditAttribution: juliangb commentedCommitted latest patch here.
Comment #40
juliangb CreditAttribution: juliangb commentedWe should now judge code style using Pareview - see #2215477: Cleanup: Resolve pareview.sh warnings.