Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replacing source code with typescript #13

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

TakNePoidet
Copy link

No description provided.

@vitalets
Copy link
Owner

Привет!
Такое мощное изменение лучше предварительно обсудить конечно, но идея переписать на ts хорошая.
Если переписывать, я бы предложил еще подумать над дизайном апи, т.к. с появлением нескольких платформ (Сбер, Маруся) стало понятно, что в текущем варианте оно не совсем удобно.

ПС: Еще в пр изменились все файлы, возможно из-за переносов строк. Это в любом случае лучше убрать.

@TakNePoidet
Copy link
Author

Очень странно почему так вышло. Поправлю.
Мне кажется, если делать общий интерфейс, то лучше с нового репозитория начать. Этот всё-таки именем привязан к Алисе.

Для чего нужен stryker? В master ветке он у меня не заработал

@vitalets
Copy link
Owner

Мне кажется, если делать общий интерфейс, то лучше с нового репозитория начать. Этот всё-таки именем привязан к Алисе.

Можно и так, да.

Для чего нужен stryker? В master ветке он у меня не заработал

Stryker хорошая тулза, позволяет прогонять мутационные тесты. Но после какого-го релиза он сломался и пока действительно не работает.

@vitalets
Copy link
Owner

В диффе еще много изменений, которые не связаны с переходом на тс.

  • дополнительные пробелы
  • переход с sh на bash
  • добавление prettier
  • итд

Это очень усложняет ревью пулл реквеста. Нужно оставить только то, что связано непосредственно с тс. А остальное лучше делать отдельными пр.

@TakNePoidet
Copy link
Author

Убрал лишнее и постарался не добавлять лишних пробелов.
У меня на wsl sh не работает к сожалению

"lint": "eslint src test",
"test": "mocha test/setup.js test/specs/*.spec.js",
"watch": "rollup -c -w",
"build": "rimraf dist/* && npx cross-env NODE_ENV=production rollup -c",
Copy link
Owner

Choose a reason for hiding this comment

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

rimraf нужно добавить в зависимости

package.json Outdated
@@ -36,21 +36,28 @@
"node": ">=8"
},
"lint-staged": {
"{src,test}/**/*.js": "eslint"
"{src,test}/**/*.(js,cjs,ts)": "eslint"
Copy link
Owner

Choose a reason for hiding this comment

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

А для чего оставляем js?

src/br.ts Outdated
* - space in tts
*/
import {textTts} from './text-tts';
import {Response} from "./reply";
Copy link
Owner

Choose a reason for hiding this comment

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

Кавычки везде лучше сделать одинарные. Можно в eslint правило добавить, если еще нет.

@vitalets
Copy link
Owner

Несколько комментов по коду написал.

У меня на wsl sh не работает к сожалению

Тогда давай это сделаем отдельным подготовительным ПР-ом.

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.

2 participants