Skip to content

Fix issue #98#101

Merged
mishanga merged 4 commits intocsscomb:devfrom
L0stSoul:dev
Nov 13, 2013
Merged

Fix issue #98#101
mishanga merged 4 commits intocsscomb:devfrom
L0stSoul:dev

Conversation

@L0stSoul
Copy link
Copy Markdown
Contributor

@L0stSoul L0stSoul commented Nov 7, 2013

Fix #98

@ghost ghost assigned mishanga Nov 8, 2013
Comment thread lib/options/vendor-prefix-align.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@L0stSoul, что такое item[2][2][1][1]?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Значение идентификатора внутри функции внутри значения идентфикатора :)
Звучит непонятно, но оно на самом деле так и есть.

Кажется что проверки перед этим на funktion и ident дают достаточно информации, если чуть чуть почитать код, я не прав?

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

В этом месте переменные и правда не нужны.
Но я знаю, что чуть ниже в этом файле есть node[i][2][1][1], и я точно так же не понимаю, что это.

Если ты добавишь комментарий к этой функции, будет здорово.
Будет очень хорошо, если комментарий ответит на вопросы:
– Что вообще возвращается? "Selector for value name" мало что объясняет.
– Почему помимо проверки на function нужна проверка на ident? Ведь до этого её не было и всем было спокойно.

проверки перед этим ... дают достаточно информации, если чуть чуть почитать код

Ты всегда читаешь чужой код целиком, внимательно и по порядку?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Да ты права, я попробую сделать адекватные комментарии.

– Что вообще возвращается? "Selector for value name" мало что объясняет.

Возвращается именно это - имя идентифактора внутри значения функции другого идентификатора либо имя идентификатора внутри значения другого идентификатора.
Я подумаю как это можно написать понятно. Наверное как то так.

Просто парсер построен таким образом что возвращает массивы. Это наверное хорошо для производительности, но портит читаемость. Плюс тема такая что мне кажется это нормально что ты сходу непонимаешь что значит node[i][2][1][1] я попробую откоментировать все что можно - надеюсь это поможет, но если честно - мне кажется намного понятнее не станет.

Ты всегда читаешь чужой код целиком, внимательно и по порядку?

Нет конечно, но речь не шла про весь и про целиком.
У меня была уверенность что если стоит item[2][2][0] === 'funktion' то понятно что item[2][2][1] это массив с содержимым функции, потому что так устроен парсер.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

если стоит item[2][2][0] === 'funktion' то понятно что item[2][2][1] это массив с содержимым функции

Ок, а что такое item[2][2]? )

Ты вполне можешь написать комментарием пример: если у нас декларация X, то эта функция возвращает Y.
Я даже думаю, что этого будет достаточно.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

пожалуй так и сделаю :)

@L0stSoul
Copy link
Copy Markdown
Contributor Author

@tonyganch я немного изменил код и учел твои пожелания, как по твоему стало понятнее?

@tonyganch
Copy link
Copy Markdown
Member

@L0stSoul, оо, очень круто. Спасибо!

Comment thread lib/options/vendor-prefix-align.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Это, пожалуй, Return property value

@mishanga
Copy link
Copy Markdown
Contributor

Cool! Rebase plz.

@L0stSoul
Copy link
Copy Markdown
Contributor Author

done!

mishanga added a commit that referenced this pull request Nov 13, 2013
@mishanga mishanga merged commit c5147c1 into csscomb:dev Nov 13, 2013
@mishanga
Copy link
Copy Markdown
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: vendor-align-prefix

3 participants