Jump to content

User:APatro (WMF)/Patch Review Feedback

From mediawiki.org

Contains a list of comments I've received on the patches that I've submitted. I've also added a few general guidelines that will help avoid silly issues such as formatting and failing tastes when submitting patches.

Some of the instructions here are specific to working with the Translate extension.

PHP

[edit]

General guidelines

[edit]
  • Always ensure you run composer fix extension/Translate before submitting patch.
  • Also run phpcs -p -s to verify any non-auto-fixable issues with the code.
  • In case of UI changes test with atleast the Timeless and Vector skin.
  • If a patch fixes multiple bugs, each bug should be listed in a separate line - Bug: T204568 \n Bug: T220789
  • When working on a big patch, keep an eye on the debug logs as sometimes warnings get logged there, and may not be shown on the user interface.
  • When adding scripts that perform an irreversible changes by default. Provide a --really flag that allows users to run a dry-run explaining what the script would do, and allow manual inspection before actually performing the operation.

Programming guidelines

[edit]
  • Use of empty is discouraged. Use isset or comparison to null to be more explicit.
  • Warnings should never be suppressed for a large section of code.
  • Throw an exception incase of unintended effects. Use specific exceptions from here - http://php.net/manual/en/spl.exceptions.php
  • Ignore parenthesis when using instanceof !$validator instanceof Validator
  • Avoid count when it's not necessary. !$matches should work well here. For eg, use if ( !$matches ) { or if ( $matches !== [] ) { instead of if ( !count( $matches ) ) {
  • Avoid isset for values that are known to be defined. Comparison to null is more explicit and avoids hard to notice errors caused by spelling mistakes.
  • If a class is in a namespace, and uses another class in a different namespace multiple prefer to add a use statement at the top of the file rather than using the fully qualified class name multiple times.
  • is_callable is safer when compared to method_exists. method_exists returns true for private functions as well.

MediaWiki

[edit]
  • Use IDatabase::addQuotes to add quotes when working with database, rather than using - "up_value <> '$sourceLanguage'"

Unit Tests

[edit]
  • Run the test cases for the Translate extension before submitting the patch - php phpunit.php --wiki=wiki /vagrant/mediawiki/extensions/Translate/tests/phpunit/
  • Use PHPUnit method - assertInstanceOf() instead of checking instanceof
  • Use setTemporaryHook when adding hooks for test cases.
  • When using @dataProvider, using yield is more readable then returning a huge array.
  • Add @covers annotation on test cases.

PHP Doc / Comments

[edit]
  • @param - The convention is to use all lowercase for primitive types and TitleCase for objects. For eg: string instead of String
  • @param / @return - Array of strings, including associative arrays can be denoted by - string[]. Same for other types of arrays.
  • Instead of writing @return void remove the @return statement entirely.
  • In the case the function declaration spans multiple lines, prefer to have the ) { is on it's own line. 
    public static function onDeleteTranslationUnit( WikiPage &$unit, User &$user, $reason,
	$id, $content, $logEntry
) {
}

  • TBD

Translations

[edit]
  • You shouldn't update the other language files besides en.json and qqq.json. Rest of the files will update automatically once the changes go through translatewiki.net.

JavaScript

[edit]
  • Run npm test in the extensions/Translate directory before submitting the patch.
  • When declaring function documentation - Please leave an empty line between the previous code / function and the comment.
  • Do not use the word extension or component. Instead use module in comments or commit messages.
  • Per Manual:Coding conventions/JavaScript#Type checks only global variables should use typeof - object properties and local variables should compare to undefined directly.
  • To allow a cookie to be changed via JavaScript, we need to set HttpOnly to false.

jQuery

[edit]
  • Use .on( 'click' ) rather than .click()
  • Use $( document.body ) instead of $( 'body' ) , and $( document.documentElement ) instead of $( 'html' ).
  • When using .get() to access elements in an array, use .get( -1 ) when accessing the last element, rather than using $focusableNodes.get( $focusableNodes.length - 1 )
Retrieved from "https://www.mediawiki.org/w/index.php?title=User:APatro_(WMF)/Patch_Review_Feedback&oldid=3613451"