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

refactor: change alert parts calculations #1084

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions senders/calc_message_parts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

3?

Copy link
Member Author

Choose a reason for hiding this comment

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

Теперь да. Между тегами, описанием и метриками

Copy link
Member

Choose a reason for hiding this comment

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

не, ты не понял, что такое три?))

Copy link
Member Author

Choose a reason for hiding this comment

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

Не думаю, что хорошая идея эту штуку в константу тащить...

Copy link
Member

Choose a reason for hiding this comment

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

messagePartsNum и коммент, описывающий

Copy link
Member Author

Choose a reason for hiding this comment

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

Поправил


if tagsLen > fairMaxLen && descLen <= fairMaxLen && eventsLen <= fairMaxLen {
Copy link
Member

Choose a reason for hiding this comment

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

давайте этот большой иф на свитч перепишем?

и условия вынесем в функции, имена которых нам будут понятно отвечеть, что это за условия?

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

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

spaceFromTags/2

2 в константу - по коду ниже она тож имеется

Copy link
Member Author

Choose a reason for hiding this comment

The 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
}
177 changes: 177 additions & 0 deletions senders/calc_message_parts_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package senders

import (
"fmt"
"testing"

. "github.com/smartystreets/goconvey/convey"
Expand Down Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The 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)
})
}
})
}
36 changes: 36 additions & 0 deletions senders/msgformat/defaults.go
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
Copy link
Member

Choose a reason for hiding this comment

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

чтобы не ломать голову почему тут +2 лучше вынести tagsStr повыше и считать длину от него


if lenTagsStr+lenTag > maxSize {
break
}

tagsStr += "[" + tags[i] + "]"
lenTagsStr += lenTag

if lenTagsStr == maxSize {
Copy link
Member

Choose a reason for hiding this comment

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

лишняя проверка как будто, предыдущая отсекает этот кейс на следующей итерации

break
}
}

if tagsStr == " " {
return ""
}

return tagsStr
}
43 changes: 43 additions & 0 deletions senders/msgformat/defaults_test.go
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, "")
})
})
}
Loading
Loading