yandex/mapsapi-codestyle

JavaScript and TypeScript Style Guide

pire_py

This is a cython-based python binding of PIRE.

What is not wrapped:

• It is impossible yet to subclass Features and Scanners in python and pass the extension back to C++;
• Most low-level operations with Fsm;
• mmap- and Action-related methods and functions;
• Run for pair of scanners;
• Feature and Encoding classes.

Interface of the binding is similar to the original one. Differences:

• All C++-space global template functions are wrapped as python instance methods.
• Fsm::operator * () is wrapped as Fsm.Iterated().
• All scanners' states are represented as classes similar to Pire::RunHelper.
• Encoding, Feature and Option abstractions are replaced with single Options abstraction, which is used to tweak Lexer behavior. Options can be either parsed from string such as "aiyu" or composed of predefined constants such as I and UTF8.
• Instead of lexer.AddFeature(Capture(42)) you use lexer.AddCapturing(42).
• Unsuccessful glue operation raises OverflowError instead of returning empty scanner.

mapsapi-codestyle's Issues

LF at the End of File

Why using this rule in 2014?
Are you still making one large js file with cat?

Move react preset to separate package

See #89 (comment)

Drop Russian translation

It seems hard to support two files and keep them the same.

Add jscs and jshint configs to codestyle repo

Возможно, имеет смысл добавить конфиги для jshint и jscs, отвечающие код-стайлу?

async without await

Should we specify function as async if it doesn't have await expression? E.g.:

async function test(): Promise<void> {
    return foo();
}

Or should use async only with await:

async function test1(): Promise<void> {
    await foo();
    return bar();
}

// Note: no async here
function test2(): Promise<void> {
   return bar();
}

Class properties order

General

In this issue I want to discuss about fields order in a class declaration.

Public, private, and protected modifiers

Because most code users or readers are first interested in the public interface, not in the implementation details, make sense use the following order: public, protected, private. But there are some drawbacks:

• Private field declarations are placed at the end of a class, but constructor (where this fields are initialized) is placed at the beginning of a class. For big classes you need often scroll code up and down each time when you want to see variable type before initialization. In other words, distance between declaration and initialization of some field can be long, but we prefer keep it short:

  Each variable should be declared as close as possible to the place where it's first used.

• TypeScript allow short field declaration:

  class Foo {
       constructor(private _bar: number) {}
  }

  instead

   class Foo {
       constructor(bar: number) {
            this._bar = bar;
       }
  
       // some code...
  
       private _bar: number;
  }

  Or allow initialize variable with declaration:

  class Foo {
      constructor() {
          // initialize other fields
      }
    
      // some code...
  
      private _map: Map<string, string> = new Map();
  }

  In this cases fields initialization is happen in different places.

Static and non-static

Should static fields be placed before or after non-static fields?

Recommendations on using `reduce`

In my opinion, we should recommend using Array.prototype.reduce alongside map and Object.keys. Code like

var obj = arr.reduce(function (obj, val) {
    obj[val.key] = obj[val.value];
    return obj;
}, {});

is much nicer than

var obj = {};
arr.forEach(function (val) {
    obj[val.key] = obj[val.value];
});

Use tslint "max-line-length" rule

Use tslint's max-line-length instead of ter-max-len

Clarifying object notation

Right now the codestyle neither approves nor forbides wrapping JS object into single line (like that: {prop1: value1, prop2: value2}, though all ‘Good’ examples contain no such code. Shouldn't we explicitly state whether or not object wrapping is appropriate?

Ternary operator

Current style does not allow easy chaining and makes it hard to visually see then/else branches when latter are longer then 20 symbols.

Long lines
(current codestyle)

var result = someLong || butNot || veryComplicated || condition ?
    some + pretty + long + but + simple + expression :
    fallback;

vs

(proposed)

var result = someLong || butNot || veryComplicated || condition
    ? some + pretty + long + but + simple + expression
    : fallback;

? and : immediately attract attention.

Chaining

Chaining ternary operator is a nice feature for small inline multi-condition choosing:

(current coodestyle)

var value = /blah/.test(x) ?
  'blah' :
  /foo/.test(x) ?
  'foo' :
  /bar/.test(x) ?
  'bar' :
  'nope';

(proposed)

var value = /blah/.test(x) ? 'blah' :
  /foo/.test(x) ? 'foo' :
  /bar/.test(x) ? 'bar' :
  'nope';

Parentheses rule

Styleguide says: Parentheses [...] Should be used only if it is required of the expression's syntax or semantics.
In my opinion, parentheses should be always use to indicate logical meaning of expression. Redundant parentheses are good.

event names

Why using event names like-that?
No major JS standard uses such naming.

Type assertions style

TypeScript supports two forms of type assertions:

1. "angle-bracket" syntax

<any>foo;

2. as operator

foo as any;

From handbook:

The two samples are equivalent. Using one over the other is mostly a choice of preference; however, when using TypeScript with JSX, only as-style assertions are allowed.

Maybe we should add some rules for that. E.g. use "angle-bracket" syntax if you don't use JSX.

Function declaration: space between function and parentheses

There are two contradicting rules:

function foo() {
    // ...
}

var bar = function () {
    // ...
};

In my opinion, there should be always a space before parentheses in function declaration.

Обязательный вызов конструктора родителя в конструкторе дочернего класса

funcion Emitter() {
  this._events = {};
}

function Child() {
  Emitter.call(this);

  // ...
}

Обязательно вызывать конструктор родителя в дочерних классах, даже если он пустой. Во-первых, если он вдруг станет не пустым, прийдётся перетрахать всех детей (добавляя этот вызов), во-вторых, не всегда вообще будет такая возможность, например, либа уже юзается в разных проектах, и вдруг в конструктор понадобилось что-то добавить. Если при этом дока либы не требовала такого вызова, то придётся извращаться в каждом методе как-то так:

Emitter.prototype.addListener = function(type, listener, context) {
  var events = this._events || (this._events = {});

  // ...
};

Хотя это может и так оказаться полезным, например, если класс должен иметь возможность использоваться как примесь, но в большинстве случаев это не удобно.
Ещё можно учесть, что иногда классы в js используются как интерфейсы. Для таких классов можно убрать это требование, но добавить какое-то требование по неймингу, например I перед именем: IDisposable.

Add rule for beginning underscore for private fields

New lines and chaining

«Операторы размещаются на предыдущей строке» — довольно тонкий момент.
Как в Яндексе принято писать чейнинг функций?

someVar
    .method1
    .method2

или

someVar.
    method1.
    method2

Ведь property accessor точно такой же оператор, как и конкатенация.

Enums code style

example:

var UpperCamelCase = {
    UPPER_CASE: 0,
    DELETED: 1,
    REED: 2
};

Let's discuss.

upd: add semi-colon.

Codestyle for ES6

Required space in class methods names

I hava this error with loris codestyle:

Missing space before opening round brace at ./examples/config.js :
     4 |
     5 |class Config {
     6 |    constructor() {
-----------------------^
     7 |        this._pagePath = path.join(__dirname, 'base-page.html');
     8 |    }

Missing space before opening round brace at ./examples/config.js :
     8 |    }
     9 |
    10 |    before() {
------------------^
    11 |        return new Promise((resolve, reject) => {
    12 |            fs.readFile(this._pagePath, (err, data) => {


2 code style errors found.

Fix table of contents in russian version

Spaces around comma

I propose adding these two rules:

"disallowSpaceBeforeComma": true,
"requireSpaceAfterComma": true

JSCS 3 support for loris preset

It has already released, but not supported by this preset

Checking whether variable is undefined

Not everyone is so happy to be able to assume that global undefined is untouched by user.
I suggest to change undefined checking to typeof x == 'undefined' since it's just safer.

Update eslint rules

New rules

es5

• unicode-bom (2.11.0) #73
• no-tabs (3.2.0) #73
• no-duplicate-case (0.17.0) #75
• ? object-curly-newline (2.12.0) #74 See #67 (comment)
• ? padded-blocks (0.9.0) #76
• ? no-unsafe-finally (2.9.0) #77
• ? no-prototype-builtins (2.11.0)

es6

• no-useless-computed-key (2.9.0) #69
• ? prefer-rest-params (2.0.0-alpha-1) #70
• ? prefer-spread (1.0.0-rc-1) #70
• rest-spread-spacing (2.12.0) #70
• class-methods-use-this (3.4.0) #71 See #67 (comment)
• no-useless-constructor (2.0.0-beta.1) #73
• no-useless-rename (2.11.0) #73
• no-new-symbol (2.0.0-beta.1) #73

Deprecated rules

• no-native-reassign -> no-global-assign (3.3.0) Change this rule later

Other

• Move no-empty-pattern from es5 to es6. #73

Using == instead of ===

=== operator possesses additional semantics in JavaScript. It means ‘compare variables values AND compare variables types’. Using it in situations when types cannot differ (like typeof a === 'string') is just overloading code with meaningless semantics.
Our rule there is as follows: if function allows polymorphic behavior, i.e. function arguments could be of different types, then all arguments MUST be explicitly cast at variables initialization block at the beginning of the function. That strictly eliminates any need of checking types below that block, and any type checking simply means that you were too lazy to cast types explicitly.
I suggest to change this rule: if you need to check types, do it explicitly at the beginning of function or code block.

Promises guide?

Since Promises will be part of the language soon, we need rules to use them, like 'reject with Error instances'.

`var` block

We prefer using single var block at the beginning of function or block. Like that:

var x = 'a',
    y = {
        z: 'x'
    };

Our arguments are:
(a) first of all, our code style says, that all function arguments casting and default values assigning must be done at the beginning of the function, like that:

function addEvent (options) {
    var type = Array.isArray(options.type) ? options.type: [ options.type ],
        context = options.context || null;
}

That, obviously, greatly increases readability and decreases risks of making casting errors.
(b) in second, every variable must be initialized.
This explicitly leads to an idea of having variables initialization block. There is no such construction in JavaScript, so we use single var to denote such block (note that we add additional indent to any objects/functions declared inside var block.

As for requirement to declare variables at the point when they're used first time, this is, in our opinion, bad architectural principle. Partly because of (a) rule, since you can't easily check whether every variable is initialized without reading entire function through, but mostly because such code is obviously under-normalized:

var x = 'a';
// code
// block
// no. 1
var y = 'b';
// code
// block
// no. 2

Obviously, such function must be split into two different functions comprising block no. 1 and block no. 2 respectively.

So, our suggestion is:

1. replace 'desclare-at-usage-point' principle with a requirement to declare and initialize every variable at the beginning of function or block;
2. use single var + additional indent to outline variable declarations block.

Add tslint config

When should a pr/issue be closed?

We have big discussions here. A good question is when we should stop and make a final decision.

Ideas? Suggestions?

Add rule for spaces in imports

Good

import {foo, bar} from './foo';

Bad

import { foo, bar } from './foo';

Like in objects and destructuring assignment.

Identifier references in object literals

I suggest to add a rule that requires use of identifiers in object literals for ES6 and TS code:

// Bad:
{foo: foo}
{
    foo: computeFoo(),
    bar: bar,
    baz: 1
}

// Good:
{foo}
{
    foo: computeFoo(),
    bar,
    baz: 1
}

Template parameter naming

There've been a discussion around our current naming rules for templates. @vmit suggests (IIUC) to adopt a rule that will allow to distinguish a template parameter in code.

The main concern is naming conflicts for constrained template parameters, i.e. the ones required to extend a type or implement a interface:

interface StyleExtractor<S extends Style> {
    (...args: any[]): S;
}

Here's S is a violation of our style guide. But it's difficult to come up with an alternative since the most sane one is Style but it's already taken:( In this particular example we can rename Style in import statement, but that's not a universal solution.

During discussion we've looked at some other codebases and code styles to gather more input. Here's the findings:

1. STL seems to use rules analogous to ours;
2. Java suggest to always name generic parameter with a single letter (but there're several violations of this rule in the JDK);
3. Google's Java style guide suggests to either use single letter names or append T to name of a parameter (but the one for C++ doesn't);
4. after quick glance TSC itself seems to use somewhat Hungarian notation, either just T, U, etc or TType.

Using Boolean constructor for type casting

Boolean constructor is not meant to be used as a cast operator.
https://developer.mozilla.org/ru/docs/Web/JavaScript/Reference/Global_Objects/Boolean
The most obvious example is Boolean(new Boolean(false)) expression, which is truthy.

Since casting to Boolean is usually done to be used in conditions, I'd prefer to use casting expression which behaves exactly the same way as condition checking in JavaScript, i.e. !!variable, though it's a bit non-obvious.

Mandatory return type for functions and methods

I suggest we add to the code style a requirement for functions and methods to always have return type specified explicitly. There're two points behind this.

1. Signature of a function (and of a method) is documentation for fellow developers. Thus, leaving it to the compiler to infer adds a burden on a human who reads your code to do the same job.
2. There's always a chance to get it plainly wrong with either a wrong guess (you meant one type, compiler inferred another) or break things later possibly in an ambiguous manner. I.e., let's say leave for the compiler to infer some types from function's return type that gets inferred itself. If it changes, compilation errors will arise from usage sights of those variables, not on call sights of the function.