Closed Bug 389536 Opened 18 years ago Closed 18 years ago

address style nits in all files located under prototypes/wcap

Categories

(Calendar :: General, defect)

Product:
Calendar
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: michael.buettner, Assigned: michael.buettner)

References

Details

Attachments

(2 files, 1 obsolete file)

shell script
1.97 KB, text/plain
Details
patch v2
845.65 KB, patch
michael.buettner
: review+
Details | Diff | Splinter Review
In light of reviewing the prototype event dialog one particular issue is to address all the style nits throughout the code. This affects all files located under m/c/prototypes/wcap.
Blocks: 370435
Flags: blocking-calendar0.7+
Attached file shell script —
This script parses a source file and dumps all the relevant locations that need to be addressed. I received this script from philipp in order to automate the task of finding all style nits, thanks again for taking care. He said that I should mention that this script is "just a hack".
The upcoming patch touches these files: sun-calendar-customize-toolbar.js sun-calendar-customize-toolbar.xul sun-calendar-event-dialog-attendees.js sun-calendar-event-dialog-attendees.xml sun-calendar-event-dialog-attendees.xul sun-calendar-event-dialog-freebusy.xml sun-calendar-event-dialog-recurrence-datepicker.css sun-calendar-event-dialog-recurrence-datepicker.xml sun-calendar-event-dialog-recurrence-preview.css sun-calendar-event-dialog-recurrence-preview.xml sun-calendar-event-dialog-recurrence.js sun-calendar-event-dialog-recurrence.xml sun-calendar-event-dialog-recurrence.xul sun-calendar-event-dialog-reminder.js sun-calendar-event-dialog-reminder.xul sun-calendar-event-dialog-timezone.js sun-calendar-event-dialog-timezone.xul sun-calendar-event-dialog.css sun-calendar-event-dialog.js sun-calendar-event-dialog.xul
I tried to come up with a list of rules that we can use to check source code against. I also would like to have an agreed set of rules that we can use for future reference as I'm still unsure what's a style rule violation and what's not. So, here we go... -> whitespace no leading whitespace on empty lines no trailing whitespace whatsoever -> intendation 4 spaces for javascript, 2 spaces for xml javascript inside xml (for bindings or such) should look like this: <method name="methodName"> <body><![CDATA[ if (true) { return 42; } ]]></body> </method> cdata-tags (opening and closing) should be placed on the same line as the enclosing tag (body-tag in this case). -> blanks blank after keywords -> if, while, for each, for blank around the following operators -> = + - * / % >= <= != blank after ',' separating function arguments -> javascript objects one property per line var object = { a: 5, b: 42 } -> brace placement place opening braces on the same line as the statement (k&r style) place the closing brace on the same column as the opening statement if (true) { while (condition) { ... } } keywords directly following closing braces should be placed on the same line do { ... } while if (true) { ... } else { ... } I intentionally did not come up with a rule such as "no line longer than 80 chars". I feel this is antiquated and shouldn't be part of this rule set. Of course, we shouldn't have excessive lenghty lines, but religiously fighting for 80 chars is something I don't want to support any longer. If anybody feels that this list is incomplete or should be corrected, please feel free to add your comment here.
Attached patch patch v1 (obsolete) — — Splinter Review
This patch applies the above mentioned rules to the list of files that comprise the event dialog. Philipp, would you please be so kind to give this patch a high priority in your review queue as this bug essentially blocks any other patch for the event dialog. Furthermore, assuming that you find remaining style nits I might have missed, I would greatly appreciate that you just post a new version of the patch. Thanks in advance for taking care of this massive patch...
Attachment #274276 - Flags: review?(philipp)
Attached patch patch v2 — — Splinter Review
I found a bunch of other things and made massive use of regexes :) I hope I didn't break anything on the way, please test before checking in. I'm requesting review to make sure its tested.
Attachment #274276 - Attachment is obsolete: true
Attachment #274792 - Flags: review?(michael.buettner)
Attachment #274276 - Flags: review?(philipp)
Status: NEW → ASSIGNED
Comment on attachment 274792 [details] [diff] [review] patch v2 I didn't find anything else to complain about -> r=mickey.
Attachment #274792 - Flags: review?(michael.buettner) → review+
I would like to note that Philipp was so kind to set up a new wiki page (see [1]) that summarizes all those rules above and adds a hell of a lot of other valuable stuff. I would like to establish this page as an established source of knowledge so that it serves as a general reference should any debate concerning style nits come up in the future. [1] http://wiki.mozilla.org/Calendar:Style_Guide
patch checked in on trunk and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
Verified in lightning (build 2007080603) and sunbird (build 20070806) -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: