refactor: change alert parts calculations #1084
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.
Тут как и в константах я не понимаю смысл оборачивать в функцию с названием просто идентичным тому
, что происходит внутри и таким большим названием :)
There was a problem hiding this comment.
с first, second, third стало еще только запутанее
There was a problem hiding this comment.
Убрал функции first..., firstAndSecond.... Вместо них написал осмысленные функции.
Да у них название длиннее, зато код читается лучше.
There was a problem hiding this comment.
52 символа в названии функции кажется перебором, функция которая назвается так, как все что внутри нее происходит бессмысленна имхо, если ты поменяешь знак внутри return, то тебе придется менять название функции :) Такая же проблема была с константами, но после переименования стало лучше, хотя бы значение с названием не так сильно стали связаны
@kissken @Tetrergeru вы как считаете?
There was a problem hiding this comment.
Для меня в целом само выражение не кажется каким-то крайне сложным и я бы оставил его так как есть, мне изначальный код с if казался более читаемым. Но вариант с функциями тоже имеет место быть, только нейминг мне не очень нравится
There was a problem hiding this comment.
я не поняла какой тут вопрос что-то :D
There was a problem hiding this comment.
Вопрос в том, что мне не нравится, что, во-первых, название функции огромное и это не читаемо, а, во-вторых, название функции просто идентично действию, которое происходит внутри и смысла от оборочивания в фукнцию я не вижу, любое изменение выражения = изменению названия функции. Хотел узнать ваше мнение на этот счет
There was a problem hiding this comment.
а, поняла, гляжу, сначала было непонятно
There was a problem hiding this comment.
да, я согласна с Данилом
а еще свитч выглядит с таким неймингом странно, показала пример поиска
| fairMaxLen := maxChars / partsCountForMessageWithTagsDescAndEvents | ||
|
|
||
| switch { | ||
| case tagsIsGreaterThanGivenLenAndOthersLessOrEqual(fairMaxLen, tagsLen, descLen, eventsLen): |
There was a problem hiding this comment.
Мне кажется, этот тот случай, когда просто выражения в условиях были бы читаемее
| @@ -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.
Мне не очень нравится то, что у нас функции передаются через переменные, немного лапшичненько. Может можно интерфейс соорудить?
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: