-
Notifications
You must be signed in to change notification settings - Fork 67
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?
Changes from 11 commits
ecb8a49
cc61504
937868e
32eebf4
88c4116
65d8624
0e56d6a
36f2097
03659d6
8f1689a
3493326
364acb4
278f458
b283a6c
fc70e5f
9b915a7
2a5ba68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,3 +14,56 @@ func CalculateMessagePartsLength(maxChars, descLen, eventsLen int) (descNewLen i | |
} | ||
return maxChars/2 - 10, maxChars / 2 | ||
} | ||
|
||
// CalculateMessagePartsBetweenTagsDescEvents calculates and returns the length of tags, description and events string | ||
// in order to fit the max chars limit. | ||
func CalculateMessagePartsBetweenTagsDescEvents(maxChars, tagsLen, descLen, eventsLen int) (tagsNewLen int, descNewLen int, eventsNewLen int) { // nolint | ||
if maxChars <= 0 { | ||
return 0, 0, 0 | ||
} | ||
|
||
if tagsLen+descLen+eventsLen <= maxChars { | ||
return tagsLen, descLen, eventsLen | ||
} | ||
|
||
fairMaxLen := maxChars / 3 | ||
|
||
if tagsLen > fairMaxLen && descLen <= fairMaxLen && eventsLen <= fairMaxLen { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Поправил. Стало лучше? |
||
// give free space to tags | ||
tagsNewLen = maxChars - descLen - eventsLen | ||
|
||
return min(tagsNewLen, tagsLen), descLen, eventsLen | ||
} else if tagsLen <= fairMaxLen && descLen > fairMaxLen && eventsLen <= fairMaxLen { | ||
// give free space to description | ||
descNewLen = maxChars - tagsLen - eventsLen | ||
|
||
return tagsLen, min(descNewLen, descLen), eventsLen | ||
} else if tagsLen <= fairMaxLen && descLen <= fairMaxLen && eventsLen > fairMaxLen { | ||
// give free space to events | ||
eventsNewLen = maxChars - tagsLen - descLen | ||
|
||
return tagsLen, descLen, min(eventsNewLen, eventsLen) | ||
} else if tagsLen > fairMaxLen && descLen > fairMaxLen && eventsLen <= fairMaxLen { | ||
// description is more important than tags | ||
tagsNewLen = fairMaxLen | ||
descNewLen = maxChars - tagsNewLen - eventsLen | ||
|
||
return tagsNewLen, min(descNewLen, descLen), eventsLen | ||
} else if tagsLen > fairMaxLen && descLen <= fairMaxLen && eventsLen > fairMaxLen { | ||
// events are more important than tags | ||
tagsNewLen = fairMaxLen | ||
eventsNewLen = maxChars - tagsNewLen - descLen | ||
|
||
return tagsNewLen, descLen, min(eventsNewLen, eventsLen) | ||
} else if tagsLen <= fairMaxLen && descLen > fairMaxLen && eventsLen > fairMaxLen { | ||
// split free space from tags fairly between description and events | ||
spaceFromTags := fairMaxLen - tagsLen | ||
descNewLen = fairMaxLen + spaceFromTags/2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spaceFromTags/2 2 в константу - по коду ниже она тож имеется There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. поправил |
||
eventsNewLen = fairMaxLen + spaceFromTags/2 | ||
|
||
return tagsLen, min(descNewLen, descLen), min(eventsNewLen, eventsLen) | ||
} | ||
|
||
// all 3 blocks have length greater than maxChars/3, so split space fairly | ||
return fairMaxLen, fairMaxLen, fairMaxLen | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package senders | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
. "github.com/smartystreets/goconvey/convey" | ||
|
@@ -33,3 +34,179 @@ func TestCalculateMessagePartsLength(t *testing.T) { | |
}) | ||
}) | ||
} | ||
|
||
func TestCalculateMessagePartsBetweenTagsDescEvents(t *testing.T) { | ||
Convey("Message parts calculating test (for tags, desc, events)", t, func() { | ||
type given struct { | ||
maxChars int | ||
tagsLen int | ||
descLen int | ||
eventsLen int | ||
} | ||
|
||
type expected struct { | ||
tagsLen int | ||
descLen int | ||
eventsLen int | ||
} | ||
|
||
type testcase struct { | ||
given given | ||
expected expected | ||
description string | ||
} | ||
|
||
cases := []testcase{ | ||
{ | ||
description: "with maxChars < 0", | ||
given: given{ | ||
maxChars: -1, | ||
tagsLen: 10, | ||
descLen: 10, | ||
eventsLen: 10, | ||
}, | ||
expected: expected{ | ||
tagsLen: 0, | ||
descLen: 0, | ||
eventsLen: 0, | ||
}, | ||
}, | ||
{ | ||
description: "with tagsLen + descLen + eventsLen <= maxChars", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 20, | ||
descLen: 50, | ||
eventsLen: 30, | ||
}, | ||
expected: expected{ | ||
tagsLen: 20, | ||
descLen: 50, | ||
eventsLen: 30, | ||
}, | ||
}, | ||
{ | ||
description: "with tagsLen > maxChars/3, descLen and eventsLen <= maxChars/3", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 50, | ||
descLen: 30, | ||
eventsLen: 30, | ||
}, | ||
expected: expected{ | ||
tagsLen: 40, | ||
descLen: 30, | ||
eventsLen: 30, | ||
}, | ||
}, | ||
{ | ||
description: "with descLen > maxChars/3, tagsLen and eventsLen <= maxChars/3", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 30, | ||
descLen: 50, | ||
eventsLen: 31, | ||
}, | ||
expected: expected{ | ||
tagsLen: 30, | ||
descLen: 39, | ||
eventsLen: 31, | ||
}, | ||
}, | ||
{ | ||
description: "with eventsLen > maxChars/3, tagsLen and descLen <= maxChars/3", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 33, | ||
descLen: 33, | ||
eventsLen: 61, | ||
}, | ||
expected: expected{ | ||
tagsLen: 33, | ||
descLen: 33, | ||
eventsLen: 34, | ||
}, | ||
}, | ||
{ | ||
description: "with tagsLen and descLen > maxChars/3, eventsLen <= maxChars/3", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 55, | ||
descLen: 46, | ||
eventsLen: 31, | ||
}, | ||
expected: expected{ | ||
tagsLen: 33, | ||
descLen: 36, | ||
eventsLen: 31, | ||
}, | ||
}, | ||
{ | ||
description: "with tagsLen and eventsLen > maxChars/3, descLen <= maxChars/3", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 55, | ||
descLen: 33, | ||
eventsLen: 100, | ||
}, | ||
expected: expected{ | ||
tagsLen: 33, | ||
descLen: 33, | ||
eventsLen: 34, | ||
}, | ||
}, | ||
{ | ||
description: "with descLen and eventsLen > maxChars/3, tagsLen <= maxChars/3", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. название не соответствует given |
||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 55, | ||
descLen: 33, | ||
eventsLen: 100, | ||
}, | ||
expected: expected{ | ||
tagsLen: 33, | ||
descLen: 33, | ||
eventsLen: 34, | ||
}, | ||
}, | ||
{ | ||
description: "with tagsLen, descLen and eventsLen > maxChars/3", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 55, | ||
descLen: 40, | ||
eventsLen: 100, | ||
}, | ||
expected: expected{ | ||
tagsLen: 33, | ||
descLen: 33, | ||
eventsLen: 33, | ||
}, | ||
}, | ||
{ | ||
description: "with tagsLen, descLen > maxChars/3, eventsLen <= maxChars/3 and maxChars - maxChars/3 - eventsLen > descLen", | ||
given: given{ | ||
maxChars: 100, | ||
tagsLen: 100, | ||
descLen: 34, | ||
eventsLen: 20, | ||
}, | ||
expected: expected{ | ||
tagsLen: 33, | ||
descLen: 34, | ||
eventsLen: 20, | ||
}, | ||
}, | ||
} | ||
|
||
for i, c := range cases { | ||
Convey(fmt.Sprintf("case %d: %s", i+1, c.description), func() { | ||
tagsNewLen, descNewLen, eventsNewLen := CalculateMessagePartsBetweenTagsDescEvents(c.given.maxChars, c.given.tagsLen, c.given.descLen, c.given.eventsLen) | ||
|
||
So(tagsNewLen, ShouldResemble, c.expected.tagsLen) | ||
So(descNewLen, ShouldResemble, c.expected.descLen) | ||
So(eventsNewLen, ShouldResemble, c.expected.eventsLen) | ||
}) | ||
} | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package msgformat | ||
|
||
import "unicode/utf8" | ||
|
||
// DefaultDescriptionCutter cuts description, so len(newDesc) <= maxSize. Ensure that len(desc) >= maxSize and | ||
// maxSize >= len("...\n"). | ||
func DefaultDescriptionCutter(desc string, maxSize int) string { | ||
suffix := "...\n" | ||
return desc[:maxSize-len(suffix)] + suffix | ||
} | ||
|
||
func DefaultTagsLimiter(tags []string, maxSize int) string { | ||
kissken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tagsStr := " " | ||
lenTagsStr := utf8.RuneCountInString(tagsStr) | ||
|
||
for i := range tags { | ||
lenTag := utf8.RuneCountInString(tags[i]) + 2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. чтобы не ломать голову почему тут +2 лучше вынести |
||
|
||
if lenTagsStr+lenTag > maxSize { | ||
break | ||
} | ||
|
||
tagsStr += "[" + tags[i] + "]" | ||
lenTagsStr += lenTag | ||
|
||
if lenTagsStr == maxSize { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. лишняя проверка как будто, предыдущая отсекает этот кейс на следующей итерации |
||
break | ||
} | ||
} | ||
|
||
if tagsStr == " " { | ||
return "" | ||
} | ||
|
||
return tagsStr | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package msgformat | ||
|
||
import ( | ||
"testing" | ||
|
||
. "github.com/smartystreets/goconvey/convey" | ||
) | ||
|
||
func TestDefaultTagsLimiter(t *testing.T) { | ||
Convey("Test default tags limiter", t, func() { | ||
tags := []string{"tag1", "tag2"} | ||
|
||
Convey("with maxSize < 0", func() { | ||
tagsStr := DefaultTagsLimiter(tags, -1) | ||
|
||
So(tagsStr, ShouldResemble, "") | ||
}) | ||
|
||
Convey("with maxSize > total characters in tags string", func() { | ||
tagsStr := DefaultTagsLimiter(tags, 30) | ||
|
||
So(tagsStr, ShouldResemble, " [tag1][tag2]") | ||
}) | ||
|
||
Convey("with maxSize not enough for all tags", func() { | ||
tagsStr := DefaultTagsLimiter(tags, 8) | ||
|
||
So(tagsStr, ShouldResemble, " [tag1]") | ||
}) | ||
|
||
Convey("with one long tag > maxSize", func() { | ||
tagsStr := DefaultTagsLimiter([]string{"long_tag"}, 4) | ||
|
||
So(tagsStr, ShouldResemble, "") | ||
}) | ||
|
||
Convey("with no tags", func() { | ||
tagsStr := DefaultTagsLimiter([]string{}, 0) | ||
|
||
So(tagsStr, ShouldResemble, "") | ||
}) | ||
}) | ||
} |
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.
3?
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.
messagePartsNum
и коммент, описывающий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.
Поправил