Please follow the following style rules when writing code, in order to minimize unnecessary back-and-forth during code review. (Note that most, but not all, of the rules on this page are baked into the default linters and the pre-push hook.)
If you use Sublime Text, consider installing the SublimeLinter, SublimeLinter-jscs and SublimeLinter-pylint plugins, following the instructions on their respective pages.
- Ensure that your code looks consistent with the code surrounding it.
- Strings should use single quotes (
'
) throughout Python and JavaScript. - Prefer having comments on their own line (above the code that's being commented on), as opposed to next to a line. The exception is when you need to disable a pylint warning for a specific line.
- The last character in each file should be a newline. (If you're using Sublime, you can enforce this locally by adding
"ensure_newline_at_eof_on_save": true
to your user preferences file.) - Avoid introducing
TODO (#XYZ): ...
comments in the files and instead try to do things correctly the first time. If you are going to add a TODO comment in any file then there needs to be (at minimum) a full comment and justification explaining what has been tried and what the issue is. The TODo should also reference an issue created on GitHub for thracking the problem.
- Avoid referencing elements of a list by a hardcoded index number, e.g.
item[0]
,item[1]
. This is because the reader typically has no idea what is significant about the element index in question. If the values in the list are of different types, consider using a domain object instead to model the item being passed around. - Avoid passing raw "dictionaries" (Python dicts or JS objects) between functions, because it's possible to add new fields to them midway through their lifecycle, which can get confusing for readers of the code. Use domain objects instead, since they have a fixed set of fields.
- Similarly, if you are passing in two lists of variables and you require both lists to be the same length (because the elements need to correspond with each other), consider using one list of composite domain objects instead.
- Avoid redefining the same variable more than once. Use different names to represent different variables, since each variable (conceptually) stores a different thing.
- Functions that start with "get", or which have GET semantics, should, under no circumstances, update or delete anything. They should be safe to call and have no side effects.
- Consider using a frozenset or tuple to a list, if the data structure is not meant to be subsequently modified. This applies especially to constants.
- If you need to raise an Exception, just do
raise Exception
-- no need to define custom exceptions. We tend to use exceptions fairly sparingly, though. - Do not use
str()
under any circumstances. Please try to usepython_utils.convert_to_bytes()
or theb'
prefix for the strings used in webapp2's built-in methods or for strings used directly in NDB datastore models. If you need to cast ints/floats to strings, please usepython_utils.UNICODE()
instead. Avoid casting strings to other types of strings using str(), unicode(), etc. Also, there should be no need to prefix any string literals with b' or u', since all string literals in Python files are prefixed withu'
by default (due to the import of unicode_literals at the top of the file). - Otherwise, please follow the Google Python style guide. In particular:
-
There should be two empty lines before any top-level class or function definition.
-
It's OK for the initial documentation string to be more than one line long.
-
Prefer string interpolation over concatenation -- e.g. prefer:
'My string %s' % varname
to'My string ' + varname
. -
When indenting from an open parenthesis ('('), prefer indenting by 4 rather than indenting from the position of the parenthesis. e.g., prefer:
my_function_with_a_really_long_name( 'abc', 'def', None)
over
my_function_with_a_really_long_name('abc', 'def', None)
-
Docstrings should start with """ and end with """. The content of each docstring should be left-aligned. For example:
"""Check whether an email message with the same recipient_id, email_subject and (cleaned) email_html_body has been sent in the last DUPLICATE_EMAIL_INTERVAL_MINS. """
Docstrings should also contain
Args
,Returns
andRaises
whenever applicable in a method. For example:def function_name(arg1, arg2): """Brief description about the function. Args: arg1: type. Short description. arg2: type. Short description. Returns: type. Short description. Raises: TypeOfException: Short description. """
-
Never use backslashes to end a line. It's hard to tell whether they're escaping newlines, spaces, or something else. Use parentheses instead to break the line up, e.g.:
my_variable = ( my_very_long_module_name.my_really_long_function_name())
-
Be careful not to use mutable objects as default values in the function or method definition. i.e., don't do things like
def foo(a, b=[]):
. -
Imports should be in three groups: standard libraries, files within the Oppia codebase, and third-party files. Each group should be separated by a single newline. Within each group, imports should be organized alphabetically. If you have additional questions, feel free to reference the Google Python Style Guide.
-
-
For pipe operations that span multiple lines, always have the pipe operator (
|
) begin on the new line.e.g., prefer:
pcoll = ( input_pcoll | "Op1" >> Operation1() | "Op2" >> Operation2() | "Op3" >> Operation3() | "Op4" >> Operation4() )
over:
pcoll = ( input_pcoll | "Op1" >> Operation1() | "Op2" >> Operation2() | "Op3" >> Operation3() | "Op4" >> Operation4() )
Note: when all pipe operations can fit in a single line, there's no need to break them up:
pcoll = input_pcoll | "Sort" >> Sort() pcoll = ( input_pcoll | "Sort" >> Sort() | "Unique" >> Unique())
General note: We use the ES2017 standard for our JavaScript/TypeScript code. (See tsconfig.json.)
-
We use extra parentheses if a statement breaks across multiple lines, similar to Python. In particular, when code in '(...)' or '[...]' spans more than one line, make a line break after the opening parentheses or bracket.
-
The indentation is always 2 spaces.
-
Try to start only function names with verbs to help distinguish them from variables. Conversely, do not start variable names with verbs.
For example:
-
For a boolean variable to check if a card is displayed:
- Correct:
cardIsDisplayed
- Wrong:
isCardDisplayed
- Correct:
-
For a function to check if a card is displayed:
- Correct:
isCardDisplayed()
- Wrong:
cardIsDisplayed()
- Correct:
-
-
We have started compiling a style guide for JavaScript. This is currently a work in progress. However, please use this as the definitive guide when figuring out the correct way to name things (CamelCase, snake_case, etc.)
-
The dependencies mentioned in strings and functional parameters of controllers, directives and factories should be in the following manner: dollar imports (e.g.
$log, $scope
etc.), regular imports (e.g.ContextService, PageService
etc.), and constant imports (e.gCOLLECTION_TAGS, DELETE_COLLECTION
etc.) all in sorted order.For Example:
oppia.thing('ThingName', [ '$sortedDollarImports', 'SortedRegularImports', 'SORTED_CONSTANT_IMPORTS', function( $sortedDollarImports, SortedRegularImports, SORTED_CONSTANT_IMPORTS) { // The implementation of `ThingName`. }]);
-
For asynchronous functions that return a promise, use the following convention:
- At the function declaration, use the keyword
async
(see here). - Add 'Async' to the function name. example:
const getUserInfoAsync = async function() { return new Promise(resolve => { setTimeout(function() { resolve("something"); }, 2000); }); }
- At the function declaration, use the keyword
-
For functions or variables that are private and that should not be exposed outside their immediate controller/service, prefix their names with an underscore (
_
) and add theprivate
keyword.
-
Make sure to follow all the javascript rules here as well.
-
Keep line lengths to at most 80 characters (with the exception of lines containing URLs, which are allowed to have a length of greater than 80 characters).
-
Declare a variable before usage. For instance:
Wrong usage:
exampleVar = true; if (someCondition) { exampleVar = false; }
Right usage:
var exampleVar = true; if (someCondition) { exampleVar = false; }
-
All loop variables should be declared. For instance:
Wrong usage:
for (item in itemList) { ... }
Right usage:
for (var item in itemList) { ... }
-
Do not add new properties to a declared variable. Ensure that all properties are declared in the variable declaration. For instance:
Wrong usage:
var person = { name: 'name', age: 'age' }; if (someCondition) { person.address = 'address'; }
Right usage:
var person = { name: 'name', age: 'age', address: null }; if (someCondition) { person.address = 'address'; }
-
Always initialize a variable at declaration. If you do not want a specific value at declaration, initialize the variable with a null value. For instance:
Wrong usage:
var person; if (someCondition) { person = 'name'; }
Right usage:
var person = null; if (someCondition) { person = 'name'; }
-
Do not overwrite the variable with a different type. Instead create a new variable whenever you have a different use case. For instance:
Wrong usage:
var person = { name: 'name', schoolName: 'school name' }; ... person = { name: 'name', officeName: 'office name' };
Right usage:
var personForSchool = { name: 'name', schoolName: 'school name' }; ... var personForOffice = { name: 'name', officeName: 'office name' };
-
If you get compilation error which says that a property does not exist on a particular type, go through the type definitions of the type and do a type casting if required. For instance:
var checkMismatch = function(searchQuery) { var isMismatch = true; $('.oppia-search-bar-input').each(function(index) { if ((<string>$(this).val()).trim() === searchQuery) { isMismatch = false; } }); return isMismatch; };
Here
$(this).val()
is type casted to a string by using<string>$(this).val()
If we do not use a typecast, typescript will give a errorProperty 'trim' does not exist on type 'string | number | string[]'
since val can be a string or a number or a string array. So, to use trim we specifically need it as a string.In many cases, you may also need to typecast to
<any>
first and then to the desired type. For example, for converting a number to string, you will need<string><any>
because neither type sufficiently overlaps with the other.You can add a new custom type definitions if type casting is not possible. In the file
typings/custom-element-defs.d.ts
, we add a new property toHTMLElement
by adding a custom type defintion. In this type casting cannot be used, since we are adding a new property to the existing type instead of changing it to some other type.
- Use
angular.mock.module
instead ofmodule
since the typings for angular-mocks does not support the usage of module. - Use
angular.mock.inject
instead ofinject
to maintain a consistent behaviour.
- If you find a missing property in a typings package, create an issue here and a new file for the custom types with the issue link in the top of the file.
- If you add a package for which type definitions are not found here, add it to
third-party-defs.d.ts
- If you add a new property on window which is not present in typings for window, add it to
custom-window-defs.d.ts
- If you add a property on scope defined in a link function, add it to
custom-scope-defs.d.ts
and add a comment specifying the filename for which it is added. - Make sure that all files have comments which explain why these custom type defintions are required and additional comments to explain each new added property if required. For example,
typings/custom-scope-defs.d.ts
has a top level comment explaining that the type defintions are needed for properties defined on scope in link function and then there are additional comments with properties added specifying which file they belong to. Go through the existing files and try to follow the same pattern when adding a new file.
Usage of old-style AngularJS directives is discouraged. Instead, use component directives. Component directives are an advanced version of AngularJS directives and are more preferred because of the "isolated scope" it creates and the reusability it offers across modules. This is also the way forward in Angular 2+.
- Do not create standalone controllers. The standalone controllers are those which are associated with the
ng-controller
directive in the HTML file. - While creating a new directive, make sure to use the component directive instead of the old style directives. Now, here's something: The component directives create what is called an "isolated scope". So the component directive can be thought of as a reusable component not dependent on its surroundings and hence "isolated". Therefore you must not use
$scope
in the directive, except for some exceptions like$scope.$on
,$scope.$apply
and other internal functions of$scope
which do not have a full replacement. Also$uibmodal
s are exempted from this rule. - There are many instances where this "isolated scope" needs to communicate with the surrounding, in such cases you must pass such data through the
bindToController
key of the component directive. This binds the values to the controller of the component directive and you can access those values in your directive's isolated scope.
In all TypeScript files in core/templates
we use webpack. That means that instead of including the required files by <script src="…"></script>
in HTML files we include them by using require(…)
in the individual TS files.
When you add new service/filter/factory dependency to service/filter/factory, you need to also require(…)
it at the top of the file.
For example if you have this filter:
oppia.filter('normalizeWhitespace', [function() {
return function(input) {…};
}]);
and need to use UtilsService
in this filter, you also need to add require('services/UtilsService.ts');
(the paths are relative to the core/templates
directory), the final filter will look like this:
require('services/UtilsService.ts');
oppia.filter('normalizeWhitespace', ['UtilsService', function(UtilsService) {
return function(input) {…};
}]);
The requires should be sorted in alphabetical order.
The rules for directives are little bit more complex. You also need to add require(…)
for service/filter/factory dependencies, but also if you use custom directive in the HTML you need to require(…)
it in the TypeScript file too.
For example, if you have directive:
require('domain/utilities/UrlInterpolationService.ts');
oppia.directive('storySummaryTile', ['UrlInterpolationService', function(UrlInterpolationService) {
return {…};
}]);
and add <sharing-links>
into the story_summary_directive.html you need to also add the new require('components/share/SharingLinksDirective.ts');
into the TypeScript file:
require('components/share/SharingLinksDirective.ts');
require('domain/utilities/UrlInterpolationService.ts');
oppia.directive('storySummaryTile', ['UrlInterpolationService', function(UrlInterpolationService) {
return {…};
}]);
The requires for directives that are in HTML are included first and separated from the regular requires by empty line.
If the file adds variable to the global scope:
// functions.ts
var functions = function() {
// something happens here.
}
We want to isolate that scope, this can be done by exporting the variable using ES6 exports.
// functions.ts
var functions = function() {
// something happens here.
}
export default functions;
And then the variable can be loaded by import functions from 'folder/folder/functions.ts';
- Do not include units if the value is 0. E.g.
margin-left: 0
instead ofmargin-left: 0px
. - Within each CSS rule, attributes should be alphabetized (e.g. 'height' before 'margin' before 'top'). This makes it easy to find the value of an attribute if there are lots of them.
- Avoid using
!important
as much as possible. - For colours, use hex values (like "#012345") or rgb(a) values, instead of names (like "white"). When using hex colour codes, try to use the 3 char version if possible (see example).
- If the CSS class is oppia-specific, prefix it with
oppia-
. This helps distinguish it from CSS classes used by other third-party libraries. - For directives, include the CSS in the directive template file, similar to what we do in this file. (Note that, in this case, all CSS rules should start with the top-level CSS class of the directive, so that they don't affect other elements outside it.) All other CSS should go in
core/templates/css/oppia.css
.
You can invoke the pre-commit script to ensure that your code follows the coding guidelines for a particular file that you've modified by running the following command from the root directory:
python -m scripts.linters.pre_commit_linter --path filepath
If you'd like to run the checks for a list of files, run the following command:
python -m scripts.linters.pre_commit_linter --files file_1 file_2 ... file_n
If you'd like to run the checks for a list of file-types, run the following command:
python -m scripts.linters.pre_commit_linter --only-check-file-extensions file_extension_type_1 file_extension_type_2 ... file_extension_type_n
If you use Sublime Text, the following settings may be useful for your "Preferences.sublime-settings -- User" file (go to Preferences > Settings)
{
"ensure_newline_at_eof_on_save": true,
"font_size": 9,
"highlight_line": true,
"rulers":
[
80
],
"shift_tab_unindent": true,
"spell_check": true,
"tab_size": 4,
"translate_tabs_to_spaces": true,
"trim_trailing_white_space_on_save": true,
"update_check": false
}