-
Notifications
You must be signed in to change notification settings - Fork 0
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
Develop #5
base: master
Are you sure you want to change the base?
Conversation
@@ -10,18 +10,22 @@ | |||
"dev": "tsc && node dist/index.js" | |||
}, | |||
"dependencies": { | |||
"@types/node": "^18.16.1", | |||
"@types/node": "^20.16.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Типизация явно должна быть в дев-зависимостях, скорее всего (вроде может быть не в дев если есть линтинг перед сборкой на стороне сервера в CI/CD)
this.bot.on('callback_query', async (ctx: CallbackQuery) => { | ||
await this.callbackHandler.handleMessage(ctx); | ||
}); | ||
|
||
this.logger.info(`Загружено ${handlerCount} обработчика событий`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Прикрути plural для склонения чисел или переформулируй сообщение в "Загружено обработчиков событий: кол-во"
let dbUser = process.env.DB_USER; | ||
let dbPassword = process.env.DB_PASSWORD; | ||
|
||
if (this.config?.database.type == DataBaseType.POSTGRESQL && dbUser && dbPassword) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Используй строгое сравнение (===) везде, где это возможно
@@ -18,11 +18,11 @@ export abstract class DataBase implements IDatabase { | |||
|
|||
abstract queryInsert(sql: string, params?: any[]): Promise<void>; | |||
|
|||
abstract getAllVoices(botId: number | undefined): Promise<CustomVoice[]>; | |||
abstract getAllVoices(botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
С таким числом аргументов лучше перейти на объект в качестве аргумента, если это возможно.
interface GetAllVoicesParams {
botId?: number;
isHidden: boolean;
...
}
abstract getAllVoices({ botId, isHidden, ...}: GetAllVoicesParams): ...
|
||
abstract getVoiceById(id: number): Promise<CustomVoice>; | ||
|
||
abstract getVoiceByTitleInclude(title: string, botId: number | undefined): Promise<CustomVoice[]>; | ||
abstract getVoiceByTitleInclude(title: string, botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Аналогично комментарию выше, тем более что аргументы схожи. Общие параметры из объекта можно вынести в переиспользуемый интерфейс и указывать его в качестве аргумента, расширяя нужными свойствами. Ну или сделать отдельные интерфейсы для аргументов функций заранее, их расширяя от общего интерфейса
if (query !== '') { | ||
this.logger.debug(`${ctx.from.username} -> ${query}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь можно свапнуть блоки инструкций местами и избавиться от отрицания в проверяемом условии. А ещё лучше избавиться от блока else вовсе, кажется, можно обойтись одним if:
`ts
if (query === '') {
...
return;
}
здесь обработка случая непустой строки
`
|
||
if (!this.config.getConfig()?.adminList.includes(msg.from.id)) { | ||
this.bot.sendMessage(chatId, 'Загрузка аудио доступна только админам') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вынести в переменную (а то и две) перед проверкой и дать человекочитаемые имена. Длина этой портянки меня пугает
const fileName: string = isAudio | ||
? msg.audio?.file_name.split('.')[0].replace(/ /g, '_') | ||
: (customSoundName?.replace(/ /g, '_') ?? randomUUID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Комментарий бы, что происходит. А действия в тернарке лучше вынести в отдельные файлы с переиспользуемыми утилитами, там дать им понятные названия и покрыть тестами. Особенно если они используются где-то ещё
getAllVoices(botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>; | ||
|
||
getVoiceById(id: number): Promise<CustomVoice>; | ||
|
||
getVoiceByTitleInclude(title: string, botId: number | undefined): Promise<CustomVoice[]>; | ||
getVoiceByTitleInclude(title: string, botId: number | undefined, isHidden: boolean, limit: number, offset: number): Promise<CustomVoice[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Аналогично комментариям про один объект в качестве аргумента. Приняли объект, вернули объект или "принцип RoRo"
export interface CustomVoice extends Pick<InlineQueryResultVoice, 'id' | 'title' | 'voice_url'> { | ||
botId: number; | ||
isHidden: boolean | number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Интересная типизация у явно булевого типа данных (исходя из названия), вы точно делаете всё правильно?)
No description provided.