Merge proposal review guideline

From Stellarium Wiki
Revision as of 13:02, 4 August 2013 by Daggerstab (Talk | contribs)
(diff) ← Older revision | Latest revision (diff) | Newer revision → (diff)
Jump to: navigation, search

These are general guidelines that should be used by members of the core team when reviewing branches proposed for merging in Launchpad. People who would like to contribute code to Stellarium should also become familiar with them, preferably before they start writing code. Changes to non-compliant code should be done before the merge, preferably by the code authors.

This document does not cover every possible issue. Use your common sense and developer experience.

(While branches are the most common way of proposing significant changes to Stellarium's code, the guidelines also apply to patches, source archives and any other possible way of submitting changes.)


General guidelines

  • New source code should follow Stellarium's coding style guidelines and conventions, including the naming of functions, classes and objects.
    • If something is not covered by the explicit rules, it's a good idea to use Qt's conventions. For an example of a more detailed C++ coding style guideline, you can have a look at this one, which includes the reasoning behind many of the conventions.
    • The only exception are third-party libraries (such as the gSat library that got included in the Satellites plug-in), but in that case, the boundary between library code and Stellarium code should be made clear. (For example, by putting the library files - and only the library files - in a dedicated subdirectory.)
  • All new source code files should have comment blocks indicating the license, including clearly indicated authorship. These should be added by the authors themselves to avoid misunderstandings. Acceptable licenses are the GNU GPL ("version 2 or later"), the GNU LGPL and the various GPL-compatible permissive licenses, such as the modified BSD license.
    The best way to avoid forgetting to add a license is just to add the header at the same time the files are created. Qt Creator has an option to automatically prepend a "license template" file to newly created files. (It can be set from "Options" -> "C++" -> "File naming" -> "License template".)
    On Linux systems, the licensecheck utility (part of the devscripts package on Debian) makes checking the licenses of multiple files very easy.
  • If the new code introduces third-party code (libraries, etc) or data, an appropriate acknowledgement should be added to the branch's version of the README file. Preferably, with the same commit that introduces the third-party code.
  • New code should be well-documented with comments. The idea is to make it easy for another developer to understand, maintain or modify the code after the original developer is gone. Ideally, there should be comments everywhere. As a minimum for approving the code, the public interface of every new class should be documented with Doxygen, including a description of the class itself (what it does and how to use it). And we mean meaningfully documented, not "void addOne(); // Adds one".
    This applies to inline comments, too. If you are unsure what to write, here are some ideas:
    • Before a block of code calculating a mathematical function: "This calculates the value of X using the formula Y found in the book Z."
    • Explain the function and contents of complex structures: "This code implements a binary searching tree where each node is..."
    • Point out design considerations (you make these, right?): "An array is used instead of a hash here, because..."
  • Check the history of the branch for bad stuff: there should be no accidentally committed build files, copyright violations or other stuff we wouldn't put in the trunk.

Code review and testing

The minimal sanity checks ("it compiles without error" and "Stellarium doesn't crash when it starts") are not enough. Try looking at the code and its behaviour for common pitfalls.

  • After running Stellarium and using the new features, look at the log file for warnings and error messages.
    This has the side effect of finding any leftover debug messages that need to be cleaned up. :)
  • The new code should be tested on all supported platforms (Windows, Linux and Mac). If the change involves the core classes and you can't test it on some platform, wait for another reviewer to do it.
  • If the code introduces new GUI strings, make sure that they are translatable (or decide whether they should be). If they are, check if the localization code has been implemented correctly - for example, formatting strings should be used instead of concatenation, because not every language has the same word order or uses the same punctuation rules.
  • Also make sure that all strings are updated when the language is changed at run time.
    Sometimes, this is easy to check just by looking at the code: find if there are any q_() calls in code that is called only once, such as a plug-in's init() method.
  • Changes to the .pot template files and .po translation files should not be committed, because resolving merge conflicts in them is painful and time-consuming. The .pot file(s) should be updated (re-generated) after the merge. Translation files for testing of new strings can be provided outside of version control (for example, by e-mail), or created by the code reviewers themselves.
  • Make sure the code doesn't duplicate already existing Stellarium features (such as StelUtils functions or vector math) and check if it uses Stellarium's API correctly.
    Of course, this requires familiarity with Stellarium's code base. ;)
  • Check if the code uses Qt correctly.
    For example, due to the way Qt's documentation is structured, a common newbie mistake is to use QCheckBox's tristate signal (stateChanged(int)) instead of the boolean signals of its parent class (clicked(bool) or toggled(bool)), causing the creation of awkward slots to deal with the signal value. Both me and Matthew Gates have done that in the past, and the same mistake was repeated more recently by the author of the Observability plug-in.
    Another mistake is not making use of Qt's features: for example, creating a custom GUI control when a convenient Qt control exists, or reimplementing QuickSort when you can just call QList::sort().
  • If the new code introduces new keyboard shortcuts or actions, make sure that the key combinations are not already in use.
  • Check how the code reacts to corrupted or missing settings or data. This is especially important for plug-ins. Ideally, a plug-in should work even if it can't save its user settings.
  • And finally, the usual good software design checks: the code should be simple and robust (as opposed to over-engineered), there should be no easily avoidable code duplication, classes should follow basic OOP principles such as encapsulation (or have very good reasons for ignoring them), the GUI layout and work flow should make sense to someone not familiar with the underlying code, etc., etc.

For reviewers

It's a good idea to keep notes while you go through the code - thus, you'll make sure that you are not forgetting anything when you post your review.

Also, when approving a merge, please, please, PLEASE provide a summary of what tests you have done and what you have reviewed, don't just select "approve". "Trust but verify." Also, this will allow junior developers to get some ideas about what doing a review should look like.

The larger and/or more drastic the code contribution, the more thorough should be the reviewing and testing.

Simplified checklist

  • Coding style and formatting?
  • Copyright headers? README update?
  • Comments? Doxygen documentation?
  • Branch history?
  • Any errors or warnings in the log?
  • Translatable strings? Dynamic re-translation on language change?
  • No duplicated features? No misused features?
  • Good design overall?
Personal tools
in this wiki
other languages