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

chore: add builder for keeping track of references found #76

Merged
merged 11 commits into from
Nov 3, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
16 changes: 2 additions & 14 deletions comments/comments.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,7 @@ func ProcessFlags(flagsRef lflags.FlagsRef, flags []ldapi.FeatureFlag, config *l
// sort keys so hashing can work for checking if comment already exists
sort.Strings(addedKeys)
for _, flagKey := range addedKeys {
// If flag is in both added and removed then it is being modified
delete(flagsRef.FlagsRemoved, flagKey)
Comment on lines -134 to -135
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deduping "modified" flags now happens in Build()

flagAliases := uniqueAliases(flagsRef.FlagsAdded[flagKey])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove duplicate or empty alias strings is now handled by Build()

flagAliases := flagsRef.FlagsAdded[flagKey]
idx, _ := find(flags, flagKey)
createComment, err := githubFlagComment(flags[idx], flagAliases, true, config)
buildComment.CommentsAdded = append(buildComment.CommentsAdded, createComment)
Expand All @@ -147,7 +145,7 @@ func ProcessFlags(flagsRef lflags.FlagsRef, flags []ldapi.FeatureFlag, config *l
}
sort.Strings(removedKeys)
for _, flagKey := range removedKeys {
flagAliases := uniqueAliases(flagsRef.FlagsRemoved[flagKey])
flagAliases := flagsRef.FlagsRemoved[flagKey]
idx, _ := find(flags, flagKey)
removedComment, err := githubFlagComment(flags[idx], flagAliases, false, config)
buildComment.CommentsRemoved = append(buildComment.CommentsRemoved, removedComment)
Expand Down Expand Up @@ -184,16 +182,6 @@ func uniqueFlagKeys(a, b lflags.FlagAliasMap) []string {
return allKeys
}

func uniqueAliases(aliases lflags.AliasSet) []string {
flagAliases := make([]string, 0, len(aliases))
for alias := range aliases {
if len(strings.TrimSpace(alias)) > 0 {
flagAliases = append(flagAliases, alias)
}
}
return flagAliases
}

func pluralize(str string, strLength int) string {
tmpl := "%d %s"
if strLength != 1 {
Expand Down
16 changes: 8 additions & 8 deletions comments/comments_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func (e *testFlagEnv) ArchivedRemoved(t *testing.T) {
}

func (e *testCommentBuilder) AddedOnly(t *testing.T) {
e.FlagsRef.FlagsAdded["example-flag"] = lflags.AliasSet{}
e.FlagsRef.FlagsAdded["example-flag"] = []string{}
e.Comments.CommentsAdded = []string{"comment1", "comment2"}
comment := BuildFlagComment(e.Comments, e.FlagsRef, nil)

Expand All @@ -205,8 +205,8 @@ func (e *testCommentBuilder) AddedOnly(t *testing.T) {
}

func (e *testCommentBuilder) RemovedOnly(t *testing.T) {
e.FlagsRef.FlagsRemoved["example-flag"] = lflags.AliasSet{}
e.FlagsRef.FlagsRemoved["sample-flag"] = lflags.AliasSet{}
e.FlagsRef.FlagsRemoved["example-flag"] = []string{}
e.FlagsRef.FlagsRemoved["sample-flag"] = []string{}
e.Comments.CommentsRemoved = []string{"comment1", "comment2"}
comment := BuildFlagComment(e.Comments, e.FlagsRef, nil)

Expand All @@ -215,8 +215,8 @@ func (e *testCommentBuilder) RemovedOnly(t *testing.T) {
}

func (e *testCommentBuilder) AddedAndRemoved(t *testing.T) {
e.FlagsRef.FlagsAdded["example-flag"] = lflags.AliasSet{}
e.FlagsRef.FlagsRemoved["example-flag"] = lflags.AliasSet{}
e.FlagsRef.FlagsAdded["example-flag"] = []string{}
e.FlagsRef.FlagsRemoved["example-flag"] = []string{}
e.Comments.CommentsAdded = []string{"comment1", "comment2"}
e.Comments.CommentsRemoved = []string{"comment1", "comment2"}
comment := BuildFlagComment(e.Comments, e.FlagsRef, nil)
Expand All @@ -228,7 +228,7 @@ func (e *testCommentBuilder) AddedAndRemoved(t *testing.T) {
}

func (e *testProcessor) Basic(t *testing.T) {
e.FlagsRef.FlagsAdded["example-flag"] = lflags.AliasSet{"": true}
e.FlagsRef.FlagsAdded["example-flag"] = []string{}
processor := ProcessFlags(e.FlagsRef, e.Flags, &e.Config)
expected := FlagComments{
CommentsAdded: []string{"| [example flag](https://example.com/test) | `example-flag` | |"},
Expand All @@ -237,8 +237,8 @@ func (e *testProcessor) Basic(t *testing.T) {
}

func (e *testProcessor) Multi(t *testing.T) {
e.FlagsRef.FlagsAdded["example-flag"] = lflags.AliasSet{"": true}
e.FlagsRef.FlagsAdded["second-flag"] = lflags.AliasSet{"": true}
e.FlagsRef.FlagsAdded["example-flag"] = []string{}
e.FlagsRef.FlagsAdded["second-flag"] = []string{}
processor := ProcessFlags(e.FlagsRef, e.Flags, &e.Config)
expected := FlagComments{
CommentsAdded: []string{
Expand Down
39 changes: 17 additions & 22 deletions diff/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,9 @@ func CheckDiff(parsedDiff *diff.FileDiff, workspace string) *DiffPaths {
return &diffPaths
}

func ProcessDiffs(matcher lsearch.Matcher, hunk *diff.Hunk, flagsRef lflags.FlagsRef, maxFlags int) {
flagMap := map[Operation]lflags.FlagAliasMap{
Add: flagsRef.FlagsAdded,
Delete: flagsRef.FlagsRemoved,
}
func ProcessDiffs(matcher lsearch.Matcher, hunk *diff.Hunk, builder *lflags.ReferenceBuilder) {
diffLines := strings.Split(string(hunk.Body), "\n")
for _, line := range diffLines {
if flagsRef.Count() >= maxFlags {
break
}

op := operation(line)
if op == Equal {
continue
Expand All @@ -69,21 +61,13 @@ func ProcessDiffs(matcher lsearch.Matcher, hunk *diff.Hunk, flagsRef lflags.Flag
// only one for now
elementMatcher := matcher.Elements[0]
for _, flagKey := range elementMatcher.FindMatches(line) {
if _, ok := flagMap[op][flagKey]; !ok {
flagMap[op][flagKey] = make(lflags.AliasSet)
}
if aliasMatches := matcher.FindAliases(line, flagKey); len(aliasMatches) > 0 {
if _, ok := flagMap[op][flagKey]; !ok {
flagMap[op][flagKey] = make(lflags.AliasSet)
}
for _, alias := range aliasMatches {
flagMap[op][flagKey][alias] = true
}
}
aliasMatches := elementMatcher.FindAliases(line, flagKey)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was always supposed to be calling elementMatcher.FindAliases - since we don't have monorepo support, it hasn't mattered so far that it wasn't

builder.AddReference(flagKey, op.String(), aliasMatches)
}
if builder.MaxReferences() {
break
}
}
flagsRef.FlagsAdded = flagMap[Add]
flagsRef.FlagsRemoved = flagMap[Delete]
}

// Operation defines the operation of a diff item.
Expand All @@ -108,3 +92,14 @@ func operation(row string) Operation {

return Equal
}

func (o Operation) String() string {
jazanne marked this conversation as resolved.
Show resolved Hide resolved
switch o {
case Add:
return "+"
case Delete:
return "-"
}

return ""
}
43 changes: 19 additions & 24 deletions diff/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ func createFlag(key string) ldapi.FeatureFlag {
}

type testProcessor struct {
Flags ldapi.FeatureFlags
FlagsRef lflags.FlagsRef
Config config.Config
Flags ldapi.FeatureFlags
Config config.Config
Builder *lflags.ReferenceBuilder
}

func (t testProcessor) flagKeys() []string {
Expand All @@ -55,21 +55,15 @@ func newProcessFlagAccEnv() *testProcessor {
flags := ldapi.FeatureFlags{}
flags.Items = append(flags.Items, flag)
flags.Items = append(flags.Items, flag2)
flagsAdded := make(lflags.FlagAliasMap)
flagsRemoved := make(lflags.FlagAliasMap)
flagsRef := lflags.FlagsRef{
FlagsAdded: flagsAdded,
FlagsRemoved: flagsRemoved,
}

builder := lflags.NewReferenceBuilder(5)
config := config.Config{
LdEnvironment: "production",
LdInstance: "https://example.com/",
}
return &testProcessor{
Flags: flags,
FlagsRef: flagsRef,
Config: config,
Flags: flags,
Config: config,
Builder: builder,
}
}

Expand Down Expand Up @@ -117,7 +111,7 @@ func TestCheckDiff(t *testing.T) {
}
}

func TestProcessDiffs(t *testing.T) {
func TestProcessDiffs_BuildReferences(t *testing.T) {
cases := []struct {
name string
sampleBody string
Expand All @@ -128,7 +122,7 @@ func TestProcessDiffs(t *testing.T) {
{
name: "add flag",
expected: lflags.FlagsRef{
FlagsAdded: lflags.FlagAliasMap{"example-flag": lflags.AliasSet{}},
FlagsAdded: lflags.FlagAliasMap{"example-flag": []string{}},
FlagsRemoved: lflags.FlagAliasMap{},
},
aliases: map[string][]string{},
Expand All @@ -145,7 +139,7 @@ func TestProcessDiffs(t *testing.T) {
name: "remove flag",
expected: lflags.FlagsRef{
FlagsAdded: lflags.FlagAliasMap{},
FlagsRemoved: lflags.FlagAliasMap{"example-flag": lflags.AliasSet{}},
FlagsRemoved: lflags.FlagAliasMap{"example-flag": []string{}},
},
aliases: map[string][]string{},
sampleBody: `
Expand All @@ -160,8 +154,8 @@ func TestProcessDiffs(t *testing.T) {
{
name: "add and remove flag",
expected: lflags.FlagsRef{
FlagsAdded: lflags.FlagAliasMap{"sample-flag": lflags.AliasSet{}},
FlagsRemoved: lflags.FlagAliasMap{"example-flag": lflags.AliasSet{}},
FlagsAdded: lflags.FlagAliasMap{"sample-flag": []string{}},
FlagsRemoved: lflags.FlagAliasMap{"example-flag": []string{}},
},
aliases: map[string][]string{},
sampleBody: `
Expand All @@ -177,8 +171,8 @@ func TestProcessDiffs(t *testing.T) {
{
name: "modified flag",
expected: lflags.FlagsRef{
FlagsAdded: lflags.FlagAliasMap{"example-flag": lflags.AliasSet{}},
FlagsRemoved: lflags.FlagAliasMap{"example-flag": lflags.AliasSet{}},
FlagsAdded: lflags.FlagAliasMap{"example-flag": []string{}},
FlagsRemoved: lflags.FlagAliasMap{},
},
aliases: map[string][]string{},
sampleBody: `
Expand All @@ -195,7 +189,7 @@ func TestProcessDiffs(t *testing.T) {
{
name: "alias flag",
expected: lflags.FlagsRef{
FlagsAdded: lflags.FlagAliasMap{"example-flag": lflags.AliasSet{"exampleFlag": true}},
FlagsAdded: lflags.FlagAliasMap{"example-flag": []string{"exampleFlag"}},
FlagsRemoved: lflags.FlagAliasMap{},
},
aliases: map[string][]string{"example-flag": {"exampleFlag"}},
Expand Down Expand Up @@ -225,7 +219,7 @@ func TestProcessDiffs(t *testing.T) {
{
name: "require delimiters - match",
expected: lflags.FlagsRef{
FlagsAdded: lflags.FlagAliasMap{"example-flag": lflags.AliasSet{}},
FlagsAdded: lflags.FlagAliasMap{"example-flag": []string{}},
FlagsRemoved: lflags.FlagAliasMap{},
},
delimiters: "'\"",
Expand Down Expand Up @@ -255,8 +249,9 @@ func TestProcessDiffs(t *testing.T) {
matcher := lsearch.Matcher{
Elements: elements,
}
ProcessDiffs(matcher, hunk, processor.FlagsRef, 5)
assert.Equal(t, tc.expected, processor.FlagsRef)
ProcessDiffs(matcher, hunk, processor.Builder)
flagsRef := processor.Builder.Build()
assert.Equal(t, tc.expected, flagsRef)
})
}
}
96 changes: 96 additions & 0 deletions flags/builder.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package flags

import (
"sort"
"strings"
)

type ReferenceBuilder struct {
max int // maximum number of flags to find
flagsAdded map[string][]string
flagsRemoved map[string][]string
foundFlags map[string]struct{}
}

func NewReferenceBuilder(max int) *ReferenceBuilder {
return &ReferenceBuilder{
flagsAdded: make(map[string][]string),
flagsRemoved: make(map[string][]string),
foundFlags: make(map[string]struct{}),
max: max,
}
}

func (b *ReferenceBuilder) MaxReferences() bool {
return len(b.foundFlags) >= b.max
}

func (b *ReferenceBuilder) AddReference(flagKey string, op string, aliases []string) {
if op == "+" {
b.AddedFlag(flagKey, aliases)
} else if op == "-" {
b.RemovedFlag(flagKey, aliases)
}
// ignore
jazanne marked this conversation as resolved.
Show resolved Hide resolved
}

func (b *ReferenceBuilder) AddedFlag(flagKey string, aliases []string) {
b.foundFlags[flagKey] = struct{}{}
if _, ok := b.flagsAdded[flagKey]; !ok {
b.flagsAdded[flagKey] = make([]string, 0, len(aliases))
}
b.flagsAdded[flagKey] = append(b.flagsAdded[flagKey], aliases...)
}

func (b *ReferenceBuilder) RemovedFlag(flagKey string, aliases []string) {
b.foundFlags[flagKey] = struct{}{}
if _, ok := b.flagsRemoved[flagKey]; !ok {
b.flagsRemoved[flagKey] = make([]string, 0, len(aliases))
}
b.flagsRemoved[flagKey] = append(b.flagsRemoved[flagKey], aliases...)
}

func (b *ReferenceBuilder) Build() FlagsRef {
added := make(map[string][]string, len(b.flagsAdded))
removed := make(map[string][]string, len(b.flagsRemoved))

for flagKey := range b.foundFlags {
if aliases, ok := b.flagsAdded[flagKey]; ok {
// if there are any removed aliases, we should add them
aliases := append(aliases, b.flagsRemoved[flagKey]...)
aliases = uniqueStrs(aliases)
sort.Strings(aliases)
added[flagKey] = aliases
} else if aliases, ok := b.flagsRemoved[flagKey]; ok {
// only add to removed if it wasn't also added
aliases := uniqueStrs(aliases)
sort.Strings(aliases)
removed[flagKey] = aliases
}
}

return FlagsRef{
FlagsAdded: added,
FlagsRemoved: removed,
}
}

// get slice with unique, non-empty strings
func uniqueStrs(s []string) []string {
if len(s) <= 1 {
return s
}
keys := make(map[string]struct{}, len(s))
ret := make([]string, 0, len(s))
for _, elem := range s {
trimmed := strings.TrimSpace(elem)
if len(trimmed) == 0 {
continue
}
if _, ok := keys[trimmed]; !ok {
keys[trimmed] = struct{}{}
ret = append(ret, trimmed)
}
}
return ret
}
4 changes: 1 addition & 3 deletions flags/types.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package flags

type FlagAliasMap = map[string]AliasSet

type AliasSet = map[string]bool
type FlagAliasMap = map[string][]string

type FlagsRef struct {
FlagsAdded FlagAliasMap
Expand Down
9 changes: 3 additions & 6 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,17 @@ func main() {
multiFiles, err := getDiffs(ctx, config, *event.PullRequest.Number)
failExit(err)

flagsRef := lflags.FlagsRef{
FlagsAdded: make(lflags.FlagAliasMap),
FlagsRemoved: make(lflags.FlagAliasMap),
}

builder := lflags.NewReferenceBuilder(config.MaxFlags)
for _, parsedDiff := range multiFiles {
getPath := ldiff.CheckDiff(parsedDiff, config.Workspace)
if getPath.Skip {
continue
}
for _, hunk := range parsedDiff.Hunks {
ldiff.ProcessDiffs(matcher, hunk, flagsRef, config.MaxFlags)
ldiff.ProcessDiffs(matcher, hunk, builder)
}
}
flagsRef := builder.Build()

// Add comment
existingComment := checkExistingComments(event, config, ctx)
Expand Down
Loading