AMO brings new levels of pedantry to Mozilla Webdev

And we love it. :)

When we first started writing AMO in PHP we agreed to follow the PEAR coding standards and left it at that. Four years and thousands of lines of code later it's roughly true, but there are some obvious mistakes and oversights. The main problem is that there is no automation for accountability. If someone happens to see something out of line in a code review, they'll point it out, but after a while everything starts to look the same. Is that brace in the right place? Is there supposed to be a space there? Minor stuff slips by, it's not a big deal.

When we moved to python we needed a new style. PEP 8 is the obvious one everyone goes with and we agreed we should follow it too. Then Jeff Balogh showed us a check script he wrote that automatically verifies code against PEP 8, PyFlakes, and complains if any lines are over 80 characters. With an automated system there is easy accountability and by checking it before you commit it's easy to stay in line.

It's not a big deal if there are spaces around your equals signs, or if there are two lines above your class declarations, but after thousands of lines of code it can make scanning and finding what you want a lot easier. The 80 character line limit is a classic programmer holy war and I think we settled on a good compromise: actual code is limited to 80 characters, templates can be more but should be a "reasonable" length. I took a screenshot of my actual workspace one day as I was digging into some code.

Click to enlarge

What do you notice about style? Aside from the Django code in the small window closest to the center, all the files fit on the screen without ugly line wrapping making it (relatively) easy to look at the full scope of MVC flow, along with the unit tests and some reference files. Open up any of our PHP files and try that and it'll just be a mess of wrapped lines or horizontal scrolling.

Code standards checks aren't something we took very seriously before but so far I'm really happy with what we've got in AMO. Automation makes all the difference.

PS: I don't have to worry about blocking out any code in screenshots - isn't open source great? Sometimes people complain about not being able to share their code and it always sounds so alien to me. I don't usually advertise on here, but if you want to share your code, we're always hiring.

5 Comments

Best part of the post is where you called the vim screenshot "IDE Screenshot".
-- Barry, 14 Apr 2010
Hi Wil. Were you able to integrate Jeff Balogh's check script with Hudson?
-- F Wolff, 15 Apr 2010
Hi Wil. Were you able to integrate Jeff Balogh’s check script with Hudson?


Not his script, but we're using the Violations plug-in with the output of PyLint which gives us close to the same thing. The script to run it is here and here is an example report
-- Wil Clouser, 15 Apr 2010
Thanks Wil. We also use pylint with the violations plug-in, but I was hoping that I can maybe configure this check script separately. The pylint output is still a bit too much, and it would be useful to have this separately.
-- F Wolff, 16 Apr 2010
Thanks Wil. We also use pylint with the violations plug-in, but I was hoping that I can maybe configure this check script separately. The pylint output is still a bit too much, and it would be useful to have this separately.


The things it will report on are about the same as pylint, but since Hudson just uses shell scripts you could have it run and if there is output email it to the committer. The output isn't in the standard violations format so you can't just plug it in to that. We use it more as a "run before commit" and "run when code reviewing" tool.
-- Wil Clouser, 16 Apr 2010

Post a comment

All comments are held for moderation; basic HTML formatting accepted.

Name: