Grouping hooks, theme functions and such;
Fixing dodgy doxygen.
Fixing comments.
Remove tabs.
Remove dodgy whitespace.
Capitalisation + termination with period.

The minor fixes that kinda crept in via an earlier commit are all in hook_menu:
Move the datetime.inc include into hook_menu.
Replaced redundant user_access statements with the available $access
Minimise a couple of drupal_get_path calls in hook_menu..

This patch will obviously break all other patches in the queue. However, all other fixes / improvements that I will be adding in the next week or so will very likely depend on this feller going in.

Thanks,
-K

Files: 
CommentFileSizeAuthor
#13 quiz_2.patch184.06 KBZen
#10 quiz_1.patch184.21 KBZen
#9 quiz9.patch171.74 KBZen
#8 quiz8.patch153.22 KBZen
#7 quiz3.patch108.1 KBZen
quiz_0.patch81.44 KBZen

Comments

Zen’s picture

oh and one more fix: The function quiz_get_user_results has a query not using the %d syntax. Thanks.

kscheirer’s picture

this would certainly be great, the function docs could really use a cleanup! You might want to move the db query change to separate patch though. It'll be a lot easier to verify this one if there are no functional changes, and then people can evaluate the query change separately.

Seems like patches are really piling up in the queue, making it hard for everyone to stay current. Anything we can do to help?

add1sun’s picture

@Zen, well unfortunately this means that one way or the other a bunch of patches will need to be rerolled. I don't have time to do it myself and Karl just spent a bunch of time rerolling all of his to make it easier to review (thanks!) and I have already begun reviewing his patches so I'm inclined at this point to put his in and then this and future patches will need to take that into account.

As for helping out, I have very limited time anymore to devote to this. There are a bunch of committers but seems like no one has time. Honestly the biggest help would be for other folks to *review and test patches*. I'm willing to commit stuff but I want to make sure it works and is sane code before I do. If folks other than the patch submitter would test/review and report back, that would do wonders for moving forward. Especially if people are doing more work for future patches so we can avoid this kind of overlap mess and duplicate effort.

Zen’s picture

I see. Well, I have deadlines to meet with this. So, I'll very likely just upload the finished article once I'm more or less done. I will, however, definitely be keeping an eye on any commits that are being made and see about reviewing and incorporating any patches in the queue.

Cheers,
-K

add1sun’s picture

Coolio. I do understand about deadlines. We'll keep plugging away on the patches and maybe some other folks will have time to step up and sort out what's what and get some nice patches out of your code too so please do keep sharing what you have.

kscheirer’s picture

If someone is able to test and verify my patches, I'll take the time to do the same on other people's patches (like this one). Even though I posted about 10, they are mostly 1 liners, and really easy to test! :)

Zen’s picture

FileSize
108.1 KB

Karl,

I intend to do so and I've had a look at a couple of them. However, my fix-it-for-production-use efforts are fast dwindling into a general rewrite of the module, mostly dealing with the dodgy UI. Therefore, a couple of the patches that apply to the rewritten bits no longer really apply any more. If you're interested, I've attached a patch detailing the status of the quiz module as of a couple of days ago. It will actually require an upgrade path for the variables table.

I will be working on it again over the weekend. The current agenda includes cleaning up the mess that is node_view, moving "take quiz" from a tab to a hook_link or perhaps sticking it inside node_view, fixing all the dodgy SQL with its comma operators, fixing direct access to question nodes and so on. Then on to the multichoice module :(

Cheers,
-K

Zen’s picture

Title: Code style + organisational fixes + minor fixes » Overhaul
FileSize
153.22 KB

Update.

Zen’s picture

FileSize
171.74 KB

update.

Zen’s picture

FileSize
184.21 KB

Update. This should be a friendlier patch format for those of you who have e-mailed me about it.

-K

wmostrey’s picture

Status: Needs review » Reviewed & tested by the community

Tested and works perfectly. One more confirmation and this gets committed.

add1sun’s picture

The patch doesn't apply cleanly to 5 anymore: 3 out of 8 hunks failed. I haven't had a chance to actually look through the whole patch yet either since it will take a bit of time. ;) I'll try to review it this week but I don't have lots of cycles to spare right now.

Zen’s picture

FileSize
184.06 KB

Update + re-roll against an updated D5. Karl's patches that were committed don't really apply to this one. Thanks for the review, Wim.

If you're looking to commit this, I'll stop adding fixes / features to this patch now. I recommend tagging the project before the commit as the upgrade path is still pending and will be an involved one - I'll tackle it in a separate issue, but will need help. This might probably warrant a 2.x release.

-K

add1sun’s picture

Status: Reviewed & tested by the community » Needs work

Hm, as I said I haven't even looked at the patch. I thought this was just cleanup/organization but I guess the size increase should have told me different. If it has new features and/or an upgrade path requirement then this needs to go into HEAD, not 5. Current 5 is just bug fixes to get a basic release out right now.

This patch should really stay as only a cleanup patch and each new feature should be in its own issue with its own patch. Wim (or anyone else) if you want this to go in, then it will need to be broken out into manageable pieces with separate issues so we know what the changes are and can assess and discuss each in turn. Even if the code doesn't get broken down right now into discrete patches, all of the new stuff going on in the code should be created as issues. This is just an important organization/workflow issue that is pretty fundamental to working on something as a group. How do I even know what I am looking at or testing for if there is no description of the problem that the code is supposedly fixing? There may also be design considerations about which path is the best way to fix certain problems. Maybe this code is not what we will use at all, but the issues started up to discuss the changes will help us keep track of what needs work and why. All of this needs to be laid out in the issue queue so we can work together on this. Also for anyone following along, here is a good article on patch management when working on a project so that you can contribute back your work in a way that the community can work with better.

Putting this back to CNW since this can't go in as is.

wmostrey’s picture

Agreed. I currently only have time to review the rest of the 'CNR' issues that can go in 5-dev (the bugfixes), I can't start splitting up this whole patch.
I will check with PHPEdu's maintainer what can be done in terms of code-recycling.

Zen’s picture

Assigned: Zen » Unassigned
Status: Needs work » Closed (won't fix)

Clearing my issue queue. I don't think these fixes are going to go in anyway - won't fixing.

-K

kscheirer’s picture

I'm still willing to take a look at these fixes and get them into HEAD,
but would it be possible to break them down into small pieces?
The last patch was 184k in size!

Or alternatively, just post the most interesting piece. Cleaning up white
space and periods is valuable, but can make it hard to evaluate a more
complicated change.