-
Notifications
You must be signed in to change notification settings - Fork 562
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
Add fields order #550
Add fields order #550
Conversation
console.go
Outdated
isOrdered = true | ||
break | ||
} | ||
} |
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.
Using a map would make this check more efficient. I often end up using simple map[string]bool
type maps so I can quickly do if _, found := myMap[key]; found {...}
type logic with efficiency.
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.
I agree and would have done it that way (and maybe saved the map on the ConsoleWriter
for reuse) but the surrounding code was just iterating in a loop (see FieldsExclude
just above) and I didn't want to break with the original author's style. Or worse yet start randomly fixing the existing loop(s).
That said, I don't mind making any reasonable change that will help the PR get accepted and merged.
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.
Understood - for FieldsOrder
is would probably make sense though for the performance benefit, especially as this is likely to be a lot larger than FieldsExclude
; but I certainly understand the concern of matching style.
Any how...just my $0.02 FWIW.
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.
Got a round toit.
.gitignore
Outdated
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.
I think you should remove this commit, because this is irrelevant with the feature we want to implement.
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.
Done
go.mod
Outdated
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.
The dependencies should not be updated by an feature implementing pull request for same reason: this is irrelevant with the feature we want to implement.
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.
Done
Could you please make this change not allocating additional memory on each write if the feature is not used? |
e921ebf
to
b32f95a
Compare
Good idea. That also cleaned up the code a lot. Not sure what I was thinking when I did all that before. :-( I stored a BTW, there's another similar situation with Both of those last two notes assume that |
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.
Please consider the suggested changes to avoid allocations.
console.go
Outdated
sort.Strings(fields) | ||
|
||
if len(w.FieldsOrder) > 0 { | ||
fields = w.orderFields(fields) | ||
} | ||
|
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.
sort.Strings(fields) | |
if len(w.FieldsOrder) > 0 { | |
fields = w.orderFields(fields) | |
} | |
if len(w.FieldsOrder) > 0 { | |
w.orderFields(fields) | |
} else { | |
sort.Strings(fields) | |
} | |
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.
Done
console.go
Outdated
if w.fieldIsOrdered == nil { | ||
w.fieldIsOrdered = make(map[string]bool) | ||
for _, fieldName := range w.FieldsOrder { | ||
w.fieldIsOrdered[fieldName] = true | ||
} | ||
} | ||
hasOrderedField := make(map[string]bool) | ||
otherFields := make([]string, 0, len(fields)) | ||
for _, fieldName := range fields { | ||
if w.fieldIsOrdered[fieldName] { | ||
hasOrderedField[fieldName] = true | ||
} else { | ||
otherFields = append(otherFields, fieldName) | ||
} | ||
} | ||
result := make([]string, 0, len(fields)) | ||
for _, fieldName := range w.FieldsOrder { | ||
if hasOrderedField[fieldName] { | ||
result = append(result, fieldName) | ||
} | ||
} | ||
return append(result, otherFields...) |
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 w.fieldIsOrdered == nil { | |
w.fieldIsOrdered = make(map[string]bool) | |
for _, fieldName := range w.FieldsOrder { | |
w.fieldIsOrdered[fieldName] = true | |
} | |
} | |
hasOrderedField := make(map[string]bool) | |
otherFields := make([]string, 0, len(fields)) | |
for _, fieldName := range fields { | |
if w.fieldIsOrdered[fieldName] { | |
hasOrderedField[fieldName] = true | |
} else { | |
otherFields = append(otherFields, fieldName) | |
} | |
} | |
result := make([]string, 0, len(fields)) | |
for _, fieldName := range w.FieldsOrder { | |
if hasOrderedField[fieldName] { | |
result = append(result, fieldName) | |
} | |
} | |
return append(result, otherFields...) | |
if w.fieldIsOrdered == nil { | |
w.fieldIsOrdered = make(map[string]int) | |
for i, fieldName := range w.FieldsOrder { | |
w.fieldIsOrdered[fieldName] = i | |
} | |
} | |
sort.Slice(fields, func(i, j int) bool { | |
ii, iOrdered := w.fieldIsOrdered[fields[i]] | |
jj, jOrdered := w.fieldIsOrdered[fields[j]] | |
if iOrdered && jOrdered { | |
return ii < jj | |
} | |
if iOrdered { | |
return true | |
} | |
if jOrdered { | |
return false | |
} | |
return fields[i] < fields[j] | |
}) |
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.
Nice!
After I did this work I saw PR 170 which is apparently languishing. My bad for not checking first. On the other hand this PR currently has no merge issues and I have time to respond to comments.