Needs review
Project:
Project
Version:
6.x-1.x-dev
Component:
Usage statistics
Priority:
Normal
Category:
Bug report
Assigned:
Reporter:
Created:
11 Dec 2008 at 20:48 UTC
Updated:
2 Jul 2010 at 22:36 UTC
Jump to comment: Most recent file
Over in #344344: Weekly usage stats not processed if we have a full week of data but we processed less than 1 week ago I noticed that the docs for project_usage_weekly_timestamp() were incomplete. I've fixed that and updated the unit tests for it.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | fix_timestamp_apis.patch | 3.29 KB | bdragon |
| #10 | fix_timestamp_apis.patch | 3.24 KB | bdragon |
| #1 | project_usage_345913.patch | 6.31 KB | drewish |
| project_usage_weekly_timestamp.patch | 1.01 KB | drewish |
Comments
Comment #1
drewish commentedpatch didn't include test changes.
Comment #2
dwwCool, thanks.
A) $weeks_ago is a wonky name for that argument now. How about renaming it to "$week_offset" instead?
B) Coding standards to format PHPdoc prefer a blank line before the @return part. So the first hunk is a regression and the 2nd hunk is a formatting regression (although the new content is good).
Comment #3
drewish commenteda) yeah, that's probably a better name.
b) not according to http://drupal.org/node/1354 was there something else you were thinking of?
Comment #4
dwwa) cool, are you going to re-roll? I have to go offline for a few hours now.
b) Bah, core is totally inconsistent about this. I think coder.module wants it this way, or something. I dunno. Anyway, I find it more readable, and I've been enforcing this "standard" for a while now, so let's leave it for now.
Comment #5
drewish commentedi can re-roll it later but right now i'm in the middle of cleaning up the charts patch.
Comment #6
drewish commentedHumm... I think it's actually a bug... project_usage_daily_timestamp() has the opposite operation for the same $days_ago style of parameter. And honestly I think we should make them consistent one way or the other.
Comment #7
dwwI think the best would be to standardize on "$(day|week)_offset" and make - past and + future. If you wanted, we could just call everything "$offset", but i think it'd be nicer to have the argument names a little more self-documenting and precise.
Comment #8
drewish commentedi'd actually prefer to go the other way and have it be days/weeks ago so that + is in the past and - the future mostly because with this code you're not going to be heading into the future looking for dates, you'd be looking backwards.
Comment #9
dwwExcept that (for example) during weekly processing, we start back at the earliest date (the last processing job) and work forward into the future for each week we haven't done yet. So, it really does seem like we use both approaches in the code, and I think a simple offset with the intuitive - == past vs. + == future behavior would be best.
Comment #10
bdragon commentedI have made changes to fix the consistency. This involved a behavioral change to project_usage_daily_timestamp(). There is one caller that uses the second param in all of contrib, and I have fixed its usage in the patch.
Comment #11
bdragon commentedOops, diffed against a subdirectory.