Skip to content
This repository has been archived by the owner on Aug 30, 2024. It is now read-only.

Code Review Criteria

Jump to bottom
Kamal Gill edited this page Feb 18, 2016 · 7 revisions

Documented here is the criteria for a pull request (PR) to pass a code review. This will help developers perform pre-flight checks before opening a PR and will provide reviewers a consistent set of criteria when examining code in a PR.

First things first

  • Features and bug fixes must be tested across Eucalyptus and AWS (unless the feature is Euca-only).
  • Features and bug fixes should be tested in the major browsers we support (IE 11, Edge, and latest Chrome, Firefox and Safari), and the browsers/devices tested on should be listed in the PR's description or the Jira ticket.
  • Features and bug fixes should pass UX review prior to code review, unless an implementation code review is requested prior to UX review.
  • Features in a PR should be only relevant to a single ticket. In a few cases, multiple related tickets may be combined in a single PR where appropriate (e.g. if a single fix spans multiple tickets), but unrelated tickets must not be combined.

Criteria for all code

  • Code should use an indentation level of 4 spaces (not tabs!).

Criteria for Python

  • Python modules must be PEP8-compliant (with line breaks at 120 char instead of 80).
  • Strings that are displayed in the interface must be marked for i18n.
  • Unused variables and imports must be removed.
  • Stray comments and print statements must be removed.
  • Unit tests must be provided for new features and should be provided for bug fixes.
  • Python-based Selenium tests should be provided.
  • Bonus points for adhering to the style guide at https://github.com/amontalenti/elements-of-python-style for conventions not covered by PEP 8

Criteria for HTML

  • HTML must be XHTML-compliant to avoid breaking i18n and potentially causing unexpected rendering problems in the browser.
  • Each id attribute for a given HTML page must be unique, meaning a page must not contain two or more elements with the same id attribute.
  • HTML tags that wrap words must be marked for translation via the i18n:translate attribute, and the tag that contains the i18n:attribute must not have child tags.
  • HTML code indentation level should be 4 spaces when possible. If a file has many levels of nesting, 2 spaces may be used for indentation, as long as the entire file is consistent.
  • Inline JS (within script tags and in HTML attributes) must be avoided.
  • References to JS files in script tags must use the minified version of the file when available.
  • HTML attributes must use double quotes, not single quotes

Criteria for JS

Criteria for AngularJS

  • AngularJS code should follow the style guide at https://github.com/johnpapa/angular-styleguide (eases transition to Angular 2)
  • Only those methods and members that are explicitly bound to HTML should be stored on $scope. The rest may be safely defined and used within the app or directive controller.

Criteria for SCSS/CSS

  • Selectors should use classes rather than ids when possible for reuse.
  • SCSS files should avoid more than two levels of nesting.
  • Inline styles (within style tags and in HTML attributes) must be avoided.
  • Existing styles should be reused or extended rather than creating new styles.

Security Criteria

  • All user-generated input that is displayed on the page must be invulnerable to Angular expression injections and should be escaped by one of three ways: Passing the input to the template as a string (for read-only display), escaping server-side via BaseView.escape_braces(), or escaping client-side via an ng-non-bindable attribute.

GitHub PR Formatting and Jira Workflow

  • The GitHub pull request must contain a descriptive title prefixed with the Jira ticket(s), and the description must contain a link to the Jira ticket. See https://github.com/eucalyptus/eucaconsole/pull/1337 for an example.
  • The Jira ticket must include a link to the pull request, and the ticket should be assigned to the reviewer via the "Submit Patch" operation.

Essential Resources

Clone this wiki locally