Skip to content

Conversation

@juwain
Copy link
Contributor

@juwain juwain commented Apr 21, 2017

No description provided.

o0 and others added 17 commits December 21, 2015 02:32
@juwain juwain self-assigned this Apr 21, 2017
@meritt
Copy link
Member

meritt commented May 26, 2017

Изометрия на фоне сломалась.

screen shot 2017-05-26 at 15 39 21

@juwain juwain requested a review from o0 July 10, 2017 09:52
Copy link
Contributor

@o0 o0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Еще где-то видел забытую инструкцию ESLint и вообще надо причесать. Я займусь!

<ul>
<li>
<a href="html-css.html">
<img src="img/logo-html.svg" alt="Логотип HTML">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML?

</li>
<li>
<a href="javascript.html">
<img src="img/logo-js.svg" alt="Логотип JavaScript">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaScript? Это же нужно для читалок, прикинь человек останавливается на ссылке, которая ведет на гайд по JS, а читалка ему «логотип JS»

Copy link
Contributor Author

@juwain juwain Jul 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Что-то мне стало казаться, что надо вообще выкинуть эту страницу. Будет две страницы: кодгайд по HTML/CSS и по Javascript. С обеих страниц будут ссылки друг на друга. Получается разводящей страницы не нужно (по крайней мере пока кодгайдов не стало много).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не знаю, я б оставил;)

javascript.html Outdated
<li>Внутри строки не используется более одного пробела.</li>

<li>Открывающие скобки блоков кода находятся на одной строке с оператором, которых их использует:<pre class="language-js"><code>
- if (condition)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну, я использовал diff из-за встроенной подсветки зелененьким и красненьким. Если у нас получилось что мы просто используем синтаксис js и ---, +++, то я переделаю на блоки с разным цветом.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я добавлю для diff стили, оказалось, что их нет по умолчанию.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не, не надо, я тут подумал, что JS с разным фоном будет лучше. Diff хак

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Окей

javascript.html Outdated

<li>В операторе <code>switch</code> запрещено дублировать условия (case)</li>

<h4>Массивы</h4>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ой 🙊 Надо что-то сюда написать или убрать заголовок.

@o0
Copy link
Contributor

o0 commented Jul 18, 2017

@zeckson читани пожалуйста файл javascript.html? Это расшифровка конфига ESLInt. Надо уже катить, сколько лет ждем;) Я хочу добавить еще строчку про приоритет версий, что желательно использовать ES6, но если совсем плохо с поддержкой, то можно ES5.

@zeckson
Copy link

zeckson commented Jul 18, 2017

Надо md закилять или синхронизировать

@o0
Copy link
Contributor

o0 commented Jul 18, 2017

Закилято!

@@ -0,0 +1,218 @@
# Стиль кода JS
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему это не удалено?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Потому что это то, что будет основной версией.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не понял. А остальные md? Или о чем это? Это что-то другое?


## Форматирование
### Строка не должна быть длиннее 80 символов
Строки более 80 символов длиной, снижают скорость чтения заставляя читатаеля
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пруф или не было!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Оке, я поищу

в разного рода презентациях, что увеличивает требования к читаемости кода

### Выравнивание знаков равенства запрещено
Трудоемкий и очень хрупкий процесс. Если хотя бы одна из переменных
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Если хотя бы одна из переменных что? Аааааа!!! Интрига!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут надо просто показать пример.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А дописать предложение? Например, давайте разберем на примере: — нельзя же так обрывать предложение на пол-фразе

+ }, true);
```

#### Аргументы функций и вызовы через цепчку отделяются четырьмя пробелами
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У нас в eslint двумя. Настало время определиться:

'indent': ['error', 2, {
      SwitchCase: 1,
      // continuation indent
      VariableDeclarator: 1, // indent is multiplier * indent = 1 * 2
      MemberExpression: 2, // indent is multiplier * indent = 1 * 2
      FunctionDeclaration: {parameters: 2},
      FunctionExpression: {parameters: 2},
      CallExpression: {arguments: 2}
    }],

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Как тебе нравится?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мне, если честно всё равно я нажимаю заветную комбинацию клавиш и просто об этом не думаю. Нужно просто синхронизировать этот документ и eslint

+ 'base-class--collapsed', this.isCollapsed())} />
```

#### Альтернативный способ переносить аргументы — выравнивать их с открывающей скобкой
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хмммм...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java-Java.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java-Java.

Автореформаттеры ломают такой способ

Правила с переносом оператора хорошо работают в языках, где необязательно
удалять забытые запятые в конце спиков. Но даже в этом случае быстрое удаление
и сортировка строк не будут работать, потому у списков будет отличаться первая
строка, а у чейнов — последняя и после сортировки и удаления нужно перепроверить
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

последняя и после сортировки и удаления нужно перепроверить получившийся список на валидность.

это про что?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

я когда писал так ощущал, а теперь не думаю вообще, что перенос точки проблема. Даже грешным делом начал подумывать удалить и разрешить все это безобразие. Как ты думаешь?

+ template.chilren[0].cloneNode(true);
```

## Правила языка
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнить — для ES без модулей

объявляется в локальной области видимости и в этом случае ее объявление
не должно затрагивать всю область видимости.

Положительный побочный эффект такого стиля заключается в том,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мне кажется это лишнее для стайлгайда. Это же не методическое пособие.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мне кажется, что почему бы и нет.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну тогда добавь такие пояснения везде. Почему так выборочно?

+ }
```

### Замыкания
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это чё?

@zeckson
Copy link

zeckson commented Jul 19, 2017

Очень плохой пулл-реквест — в нём свалено всё и изменения и рефакторинги и правки и черт в ступе. Пожалуйста не делайте так. Никакого хорошего ревью не получится

@zeckson
Copy link

zeckson commented Jul 19, 2017

Я хочу добавить еще строчку про приоритет версий, что желательно использовать ES6, но если совсем плохо с поддержкой, то можно ES5.

Давай не будет добавлять такую строчку.

@meritt
Copy link
Member

meritt commented Jul 19, 2017

Я согласен с @zeckson. Сейчас это не вмёржить уже. Можно было сделать ветку от этой ветки.

@o0
Copy link
Contributor

o0 commented Jul 19, 2017

Ну если только по тексту то надо было обсуждать в другой ветке, это уже версия, которую @juwain готовит к публикации. Может быть да, можно было сделать в несколько шагов, но сейчас уже PR какой есть, надо что-то делать. Ок, плохо, выход какой? Никаких альтернатив не предложено. Повздыхали и разошлись.

@o0
Copy link
Contributor

o0 commented Jul 19, 2017

Давай не будет добавлять такую строчку.

Аргументы и альтернатива. Давай не добавлять такую строку потому что это ложь, а вместо этого лучше напьемся

javascript.html Outdated

<li>Блоки кода отделяются друг от друга не более чем двумя пустыми строками</li>

<li>В качестве символа строки используется стандартный символ, который используется в системе</li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это чё?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EOL. LF/CRLF и вот это все.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тогда нужно написать в качестве символа переноса строки

javascript.html Outdated

<li>В блоках кода первая и последняя строка не должны быть пустыми (код не отбивается от начала блока кода)</li>

<li>После двоеточий и точек с запятым ставятся пробелы. Перед ними — не ставятся.</li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Где-то есть точки в конце, где-то нет — лучше определиться

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Долой точки в конце

javascript.html Outdated

<li>При объявлении вычисляемых ключей в объектах с помощью синтаксиса ES2016 в квадратных скобках не используются пробелы</li>

<li>Оператор вызова функции () не отделяется круглыми скобками от названия функции</li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Имется ввиду вот это: (callFun)()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Забыл вопрос задать =)
Имется ввиду вот это: (callFun)()?

javascript.html Outdated

<li>Перед скобками начинающими новый блок кода должен ставиться пробел.</li>

<li>при объявлении анонимных функций, скобки с параметрами ставятся непосредственно после ключевого слова function. Если функция именована, перед названием ставится пробел, после — нет.</li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

С большой буквы

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В нашем eslint это не так.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeckson давай договоримся, что если ты что-то отрицаешь, то ты делаешь противопоставление, так удобней. Да, я мог ошибиться, но я не в контексте. Будет конструктивней, если ты напишешь: у нас не так, а вот так.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o0 хорошо, у нас вот так:

'space-before-function-paren': ['error', {named: 'never', anonymous: 'always'}],

javascript.html Outdated

<li>В spread-операторе точки не отделяются от названия коллекции</li>

<li>Звездочка после ключевого слова yield не отбивается пробелом. После звездочки проблел ставится всегда.</li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

проблел


<article class="chapter-part">
<div class="chapter-part-col chapter-part-col--full-width">
<h3 id="">Неиспользуемый код</h3>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Закомментированный код?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

А?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну про закомментированный код или это не сюда?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не, про закомментированный код у нас нет ничего в этом гайде. ESLint же не умеет его проверять.

javascript.html Outdated
</ul>

<ul>
<li>В конструкции <code>try..catch</code> запрещен пустой блок <code>try</code></li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пустой блок catch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javascript.html Outdated
<ul>
<li>При итерировании по объектам через <code>for..in</code> при работе со свойствами используется конструкция <code>hasOwnProperty</code></li>

<li>В функциях не используются обращения к <code>caller</code> и <code>callee</code></li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Перенести в запрещенное

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Где у нас запрещенное?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javascript.html Outdated
<article class="chapter-part">
<div class="chapter-part-col chapter-part-col--full-width">
<ul>
<li>При итерировании по объектам через <code>for..in</code> при работе со свойствами используется конструкция <code>hasOwnProperty</code></li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for..in не используется

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Правило?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

увы в eslint нет такого правила, но я бы запретил совсем

<pre class="language-js language-correct"><code>const result = 2;
</code></pre>
<br>
<pre class="language-js language-incorrect"><code>switch (val = getVal(), val) {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

это и так запрещено выше

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Потерял контекст

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

нельзя присваивать и использовать одновременно

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это немного не то!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Разные правила, каждый пример взят по отдельному правилу, я не вспомню сейчас названия обоих. Это правило не про присвоение и использование, а про оператор запятая в условиях.

@zeckson
Copy link

zeckson commented Jul 19, 2017

Аргументы и альтернатива. Давай не добавлять такую строку потому что это ложь, а вместо этого лучше напьемся

Без аргументов и альтернативы, просто предложение. Т.к. добавление такой строки приведет к дальнейшему бесконечному рефакторингу многочисленных мест.
Альтернативно я бы просто сделал сноску напротив всех пунктов где есть раздвоение, например ES2015 и выше

@zeckson
Copy link

zeckson commented Jul 19, 2017

Ок, плохо, выход какой? Никаких альтернатив не предложено. Повздыхали и разошлись.

Я отревьюил как смог, но html форматирование мешает сосредоточиться на тексте к сожалению. Формат html плохо подходит для редактуры на мой взгляд. Плюс он не подразумевает модульности

@o0
Copy link
Contributor

o0 commented Jul 19, 2017

@zeckson согласен.

<h3 id="">Операторы</h3>
<ul>
<li>В конструкторе классов-наследников обязательно вызывается <code>super()</code></li>
<p>В конструкторе классов-наследников обязательно вызывается <code>super()</code></p>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я повторю свой вопрос — нужно ли дублировать синтаксические ошибки в стайлгайде?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хотя ладно, пусть будет


<li>В конструкторе нет обращения к <code>this</code> до того, как будет вызван <code>super()</code></li>
</ul>
<p>В конструкторе нет обращения к <code>this</code> до того, как будет вызван <code>super()</code></p>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я повторю свой вопрос — нужно ли дублировать синтаксические ошибки в стайлгайде?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Хотя ладно, пусть будет

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Правило ESLint! Тулза проверяет, хороший ли синтаксис, у них есть такие правила, например проверка валидности регулярки. Если регулярка невалидна, то код упадет, но ESLint не позволяет до этого дойти, так что, я считаю, что это хорошо.

@o0 o0 merged commit 290487b into master Aug 1, 2017
@juwain juwain deleted the feature/total-destructuration branch August 23, 2017 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants