Author Archives: allyshiasewdat

Why you should lint your code

For this blog post, I thought I’d share some thoughts on a tool I worked with during this term while contributing to Review Board. I’ll understand if you prefer to skip to the last section for the essentials : my impressions on working on Review Board with UCOSP this term 🙂

Review Bot and static analysis

My focus on Review Board was on Review Bot, an extension to Review Board that automates various tasks on uploaded code in a Review Board review request, then publishes its output as a review. One main focus of Review Bot at the moment is running static code analysis on code submitted in a review request, i.e. running a style checker such as PEP8 (for Python code) and outputting style errors found. My contributions were thus in the same vein, and my task was two fold : 1) Add another static analysis tool : in particular, the JSLint JavaScript code quality tool; 2) Add functionality to allow manual triggering of Review Bot’s static analysis (rather than only allow its automatic trigger on review request publication).

For the first, I essentially had to add a JavaScript engine as a dependency to Review Bot so that we could run JSLint (written in JavaScript) on our code. I then ported JSLint itself into our project and plugged it into our existing tool framework so that it could be triggered along with PEP8. The second task is in progress, but it involves adding some new elements to the UI and tweaking the Review Bot extension such that it could be triggered manually from the Review Board GUI, taking into account user options and permissions that should be set to arbiter the running of scripts on code uploaded to Review Board.

But why static analysis?

In this context – Review Board is a code review tool after all – the answer appears fairly obvious : production-quality code should be tidy and conform to certain standards before it gets merged into the codebase, and a quality verification process should be part of the review. Static analysis of code – “linting” in the case of JSLint and other similar tools – is an excellent tool in ensuring quality. Human errors are not uncommon, and even if the compiler doesn’t complain, there are style errors that can actually lead to defects. Another point to ponder: a static analysis tool will complain if certain variables are undeclared, if variable names are misspelled, if the code isn’t formatted consistently, among other things. If a static analysis tool is having trouble deciphering your code, consider that it might be difficult for a fellow human to understand it as well. This is where static analysis helps us write quality code that not only performs well, but less painful to read in pair or group programming situations.

Case in point : JavaScript. It is a forgiving language; some might call it sloppy. The standard JavaScript interpreter won’t enforce variable declaration, won’t ensure that a method call respects the actual method signature, won’t complain if variable nomenclature is inconsistent, won’t require that constructors begin with an upper case letter, allows overloading of the “+” operator, allows type coercion (ie automatically converts one type to another) … we get the idea. So how do we ensure that our JavaScript is still valid and polished at the end of the day? Enter JSLint.

JSLint by Doug Crockford

Doug Crockford is a JavaScript enthusiast, as it appears from his writings on the matter. He is a believer that JavaScript, despite it’s more easy-going and Web-related nature, is not in fact a tool for amateurs, but a capable and professional-grade language that is suitable for all sorts of purposes other than dynamic Web page functionality (see his article on JS being a misunderstood language). He is also weary of the mistakes that programmers can easily make in JavaScript, mistakes that would be permitted based on the ECMAScript Programming Language Standard, but that just won’t fly in a professional context. Hence his “professional subset” of JavaScript defined by his code quality tool JSLint.

JSLint is strict. It won’t tolerate missing semi-colons, neither does it like variables defined in blocks. If you are writing JavaScript, according to JSLint, you must ensure that each line (one instruction per line only) is terminated by a semi-colon so as to prevent ambiguity, and you must declare all variables at the top of a function because there are just too many scope issues if you don’t. Also, variables must be declared using the var keyword, lest an undeclared variable become an implied global variable. If statements must be written using { } brackets, rather than take advantage of of the following allowed syntax :

if (condition)

statement

as that can easily be misinterpreted by another coder. Constructors used with the new prefix must be capitalized, or else if the new keyword is omitted, a lack of capital letter convention could lead to a call to the wrong function. The ++ and — operators must be avoided because a syntax error such as print(“Hello” + + name) could be mistaken for a legitimate operation, and bitwise operators must be avoided because they are not as efficient as they are meant to be in the first place (because of conversion from floating point to integer and back), and secondly their resemblance to boolean operators (see &, and I) make them prone to error. So Crockford says no. JSLint also says no to the “eval()” function, which is often a security hole if not used carefully as it can execute a (potentially malicious)user-provided string to the application’s (or user’s) peril.

These are just the points that stood out : a complete list is available and explained in the JSLint documentation. It seems so far that however restrictive these rules seem to be, they are backed up with valid arguments that identify potential (and dangerous) programming errors that could easily go uncaught or worse, be propagated (think very large code base and/or very many developers). What’s more, ALL of the above requirements can be turned off via JSLint’s options. However, some still argue that JSLint might go too far.

When the code quality tool is … a tyrant ?

Anton Kovalyov and Paul Irish wrote JSHint : a popular fork of JSLint. Kovalyov himself, a past content JSLint user, describes that eventually Crockford’s tool became “increasingly opinionated and hostile towards your code”. His view is that JSLint is looking less and less like a defect-prevention quality tool and more and more like a way to coerce developers into writing JavaScript the way Crockford would like it. He provides a hard-to-refute example in which the following construct would be forbidden :

for ( var name in obj ) {

}

This would be prohibited by JSLint because the tool requires variables to be declared at the beginning of functions only, with an extended preference that only one “var” declaration be made (forcing all variables to be declared in one place). While the latter requirement can be switched off using JSLint’s options, the former (and main) rule cannot. This turns off some (a lot of?) developers who would rather allow some flexibility for constructs like in the above example.

To that point, one particular other restriction in the JSLint documentation stood out : underscores are forbidden as the first letter in variable names. While it is not uncommon to use the underscore prefix in nomenclature to distinguish “private” variables (not an official notion in JavaScript), Crockford says this does not actually ensure privacy (he is right) then adds “Avoid conventions that demonstrate a lack of competence.” Ouch. On the bright side though, even this restriction can be turned off using options.

The response, JSHint, is a tool that allows the developer more flexibility to determine their own coding conventions before going into static analysis. One of the main problems developers who used JSLint seemed to have was that JSLint would stop short of scanning the entire file after having met what appears to be a variable quota of errors. JSHint takes away that problem (and it’s a pretty big one). I have never used JSHint so I cannot comment, but I’d be curious to see the difference for myself.

Conclusion?

It’s worth mentioning that JSLint’s source contains the following comment:

“//WARNING: JSLint will hurt your feelings”. I suppose it’s meant to be aggressive and intolerant, as that is precisely what Crockford appears to be after: ensuring poor programming habits don’t create unintended errors, and that coders can focus on writing code and not on scrutinizing their own styling mistakes, since the tool rigorously does it for them. That error quota is quite bothersome, but I can appreciate the intention here: style and syntax matter when it comes to preventing avoidable problems down the line.

Enough rambling about JavaScript: why everyone should do UCOSP

Back at university, where I study Computer Science, “the craft” is generally reduced to a theoretical subject. A lot of the coding assignments from day one were to be done in Java, and the extent to which we study Web frameworks and other Web technologies is minimal. Sad as this is, there is hope. Once in a while, the opportunity to work on an exciting project like Review Board comes along and rocks the worlds of those who participate, by allowing them to explore and experiment with new technologies and environments, learn to overcome real new challenges that help build new skill sets, and contribute to code that matters to real users. As much as I might sound like every other Computer Science who has complained, I cannot emphasize enough how important it has been for me to break out of the comfort of the classroom during my first degree and experience coding on a larger project such as this one. Given the opportunity to participate in the Undergraduate Capstone Open Source Projects (UCOSP) for my final honours project, I jumped at the possibility to be part of a new team and collaborate with creative minds. Needless to say, I have no doubt it was the right thing to do. The many new things I’ve learned during this term so far – from becoming comfortable in a Linux environment to learning a new programming language (Python) new Web framework (Django) – have made this an invaluable experience.

As the semester comes to an end, hard as it is to believe, these are in fact closing remarks on my Review Board experience. This was an immensely rewarding and educational experience for me, and I am a better developer now because of it.

Status Report — November 4th, 2012

Wesley Ellis

Currently:

  • Getting ReviewBot to pass parameters to buildbot

Next:

  • Get a try scheduler running for buildbot
  • Configure options

Questions:

  • How should password storage work? I will need to ask the user to input their buildbot username/password (unless they have ssh passwordless auth setup) but I want to make sure I’m storing that information properly

Jesus Zambrano

Currently:

  • Looking into preventing a page reload for the publish button of the draft-banner.

Next:

  • Make necessary changes in reviews.js and datastore.js and submit for review.
  • Address comments in r/3389.

Allyshia Sewdat

Currently:

  • Addressing comments/issues in Review 3435
  • Trying to polish off the JSLint plugin by making sure options are working properly

Next:

  • Take a look at the new UI for manually triggering static analysis when a review is posted. Will start with some mockups to get feedback on what the new UI should look like.

Aamir Mansoor

Currently:

  • Looking at Christian’s review request that allows filetype registration support (r/3441)
  • Going through comments left by Karl

Roadblocks:

  • It would be great if one of the mentors could look at my review request (r/3434) so I can find out and start addressing issues with the code I have so far

Next:

  • Applying Christian’s changes on a branch and rebasing my code on top of it
  • Addressing any comments/issues on r/3434

Karl Leuschen

Currently:

  • Removed WIP status from earlier review request (r/3423).
  • Preparing a new WIP request for a key association field for the repository form.
  • Reviewing r/3434

Next:

  • Get feedback on new WIP request and try to finish off changes to repository form.

Tina Yang

Currently:

  • Get binary files to display inline with help from the suggestions from the review request (r/3457), specifically using template tags instead of diffutils function to fetch the binary files now.

Next:

  • Submit the revised code for review and get started on some tests.

John Sintal

Currently:

  • Working on rb patch.

Roadblocks:

  • Not being able to test on my dev server (not able to post requests).

Next:

  • Finish rb patch. Steven answered some of questions that should help me move forward.
  • Fix why I can’t test on my dev server the

Michelle Chuang

Currently:

  • Going through and applying Christian’s patch for file attachments (/r/3441/)

Next:

  • Continuing to work on displaying two images side-by-side before trying to add different view modes

Sampson Chen

Currently:

  • Reading over the code in reviewboard/extensions/*, djblets/extensions/*, reviewboard/reviews/ui/*
  • Getting a better understanding of how hooks in RB and Django entry point work.

Roadblocks:

  • None at the moment.

Next:

  • Post a WIP review-request as I write the code for ReviewUI integration with extensions + check-in with mentors every so often to make sure I’m headed in the right direction.

Questions:

  • None at the moment.

Meeting Minutes for October 28th, 2012

Announcements:

  1. Midterm evaluations have gone out, and you should have received them by email. If you haven’t, please let Mike C. know ASAP.
  2. We’re aiming for 1.7 RC this week, and a full release early November. As such we are limiting what goes into master during this time in order to maintain stability. Much of what’s being written this semester will target a 1.7.x release.

Questions / Answers / Tips:

Q (Tina): What’s the best way to commit fixes to issues on review requests? Is one WIP review request okay (to maintain context), or if it gets too big should we have multiple ones with different branches like : “main-project/sub-task”?

A: For a fix within one change, just amend that change, rather than have a new commit. Save new commits for whole pieces of work. In the end we just apply a patch so it won’t matter much for us. Breaking up a project into smaller, atomic pieces can be good and make reviewing go faster. One way or another, remove “WIP” when a change is ready for committing.

Regarding branching: you should group review requests and branches into logical pieces of work (tasks). If a task is done, create a new branch for the next one, and use parent diffs if you need to for RB.

Q (Wesley): How to get access to BuildBot in order to test ReviewBot plugins?

A: You should set up your own BuildBot. The one we have is very specialized. We’re not in a place to be able to grant access, and we don’t have sandboxing anymore.

Q (Sampson): Is my next project confirmed as “ReviewUI integration with extensions infrastructure”?

A: Yes, let’s start on that. It won’t be a very large one, but we can go from there on some features.

Q (Michelle): Are file attachments attached to a review request in general or to a specific diff?

A: Right now, just in general. With Tina’s work (this semester), both.

Q (Michelle): Should we be diffing file attachments that differ between diffs? I recall a few weeks ago you had said that we should be diffing between file attachments with the same name, but you can’t attach file attachments with the same name to the review request in general.

A: There is no infrastructure for that today. It would have to be written at some point. Right now the focus is on display/review for individual files, and on image diffing, which will both be super useful and serve as a good test for the file attachment diffing work.

Q (Karl): Will the 1.7RC change the way we work in the short term? Do we just pull from master as usual?

A: There should be no effect; just keep pulling from master.

Q (Jesus): A server error message appears and disappears when the discard review request button is clicked. The message quickly goes away and the page renders fine. Any suggestions? (NB: Error is visible in server console but disappears as well. Browser is Firefox.)

A: Try reproducing in Chrome. First open up the Network Log in the Developer Tools and click the black circle button in the bottom. Reproduce it. It should keep the HTTP request for that around, and from there, you should be able to look at the result.

Note: Developer console in Firefox will also log and report HTTP requests.

Q (John): Anyone have tips on how to force (require) cmd args for an rb command? Right now the command would be called like this: rb patch -r <request_id> [–diff-revision=<diff_revision>]. We want to call the command like this: rb patch <request_id> [<diff_revision>].

A: Look at postreview.py. After you parse options, you should look at the remaining argument list and ensure you have what you need. Instead of sys.argv, you’re going to access the args coming from parse_args. Those will be the remaining arguments after all –flags have been considered.

Q (Allyshia): If we want to review other peoples’ code, we just head on over to the “reviewboard” group at reviews.reviewboard.org, pick off a review request that appears to be from one of us and start commenting?

A: It doesn’t even have to be from one of the other students. But otherwise, yes.*

* See tips on reviewing below.

Q (Allyshia): Will the 1.7 RC changes affect those working on ReviewBot in any way?

A: They shouldn’t, no.

General Tips:

Regarding reviewing:

  • We’re not expecting you to become experts on the stuff that you’re reviewing in order to find defects – just take a look, and if you see something odd, bring it up. If something is not clear, ask about it. Style problems are valid issues too.
  • Alternatively, you don’t just need to find defects. If you see a particularly “beautiful” piece of code that you think is awesome-graceful, point it out and tell the author that it’s cool!

Regarding mid-term evaluations:

  • Remember that the midterm evaluation is more of a sign-post, and is not being used for your final grade. It’s more of a way to let you know how you’re doing, though final grades will be computed in a similar manner. Feel free to approach mentors if you have any questions.