-
Notifications
You must be signed in to change notification settings - Fork 68
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
refactor: change alert parts calculations #1084
base: master
Are you sure you want to change the base?
Conversation
ATTENTION: - slack tests failed - need to refactor tg message formatter
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1084 +/- ##
===========================================
- Coverage 66.85% 52.64% -14.21%
===========================================
Files 219 287 +68
Lines 12778 16593 +3815
===========================================
+ Hits 8543 8736 +193
- Misses 3684 7303 +3619
- Partials 551 554 +3 ☔ View full report in Codecov by Sentry. |
senders/calc_message_parts.go
Outdated
} | ||
} | ||
|
||
func firstIsGreaterThanGivenLenAndOthersLessOrEqual(givenLen, first, second, third int) bool { |
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.
Тут как и в константах я не понимаю смысл оборачивать в функцию с названием просто идентичным тому
, что происходит внутри и таким большим названием :)
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.
с first, second, third стало еще только запутанее
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.
Убрал функции first...
, firstAndSecond...
. Вместо них написал осмысленные функции.
Да у них название длиннее, зато код читается лучше.
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.
52 символа в названии функции кажется перебором, функция которая назвается так, как все что внутри нее происходит бессмысленна имхо, если ты поменяешь знак внутри return, то тебе придется менять название функции :) Такая же проблема была с константами, но после переименования стало лучше, хотя бы значение с названием не так сильно стали связаны
@kissken @Tetrergeru вы как считаете?
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 казался более читаемым. Но вариант с функциями тоже имеет место быть, только нейминг мне не очень нравится
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.
я не поняла какой тут вопрос что-то :D
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.
Вопрос в том, что мне не нравится, что, во-первых, название функции огромное и это не читаемо, а, во-вторых, название функции просто идентично действию, которое происходит внутри и смысла от оборочивания в фукнцию я не вижу, любое изменение выражения = изменению названия функции. Хотел узнать ваше мнение на этот счет
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.
а, поняла, гляжу, сначала было непонятно
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.
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.
Вернул условия, предлагаю остановиться на этом варианте
@@ -27,6 +26,10 @@ type EventStringFormatter func(event moira.NotificationEvent, location *time.Loc | |||
// DescriptionCutter cuts the given description to fit max size. | |||
type DescriptionCutter func(desc string, maxSize int) string | |||
|
|||
// TagsLimiter should prepare tags string in format like " [tag1][tag2][tag3]", | |||
// but characters count should be less than or equal to maxSize. | |||
type TagsLimiter func(tags []string, maxSize int) string |
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.
Мне не очень нравится то, что у нас функции передаются через переменные, немного лапшичненько. Может можно интерфейс соорудить?
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.
Интерфейс соорудить можно, но тогда он будет выбиваться из остальной кучи типов, представляемых функциями...
Соответственно можно попытаться и остальные Formatter
-ы и т.п. на интерфейсы заменить, но предлагаю это отдельным PR-ом делать, а то этот уже внушительный получился...
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.
Ок, мб заведёшь задачу?
Change the rule for calculating alert parts if they are too big
There is no limits on tag length and tags count, so users can use a lot of long tags. In such case the function that resizes alert parts got bad data, which results to wrong calculations and panic in notifier.
This PR offers new function for resizing alert parts. Now the space is distributed between tags, trigger description and events block. If possible, space is given to description and events.
This PR affects the following senders: