Repository navigation
Add fields order - #550
Add fields order#550
Conversation
| isOrdered = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
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.
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.
Got a round toit.
There was a problem hiding this comment.
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.
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.
|
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 |
rs
left a comment
rs
left a comment
There was a problem hiding this comment.
Please consider the suggested changes to avoid allocations.
| sort.Strings(fields) | ||
|
|
||
| if len(w.FieldsOrder) > 0 { | ||
| fields = w.orderFields(fields) | ||
| } | ||
|
|
There was a problem hiding this comment.
| sort.Strings(fields) | |
| if len(w.FieldsOrder) > 0 { | |
| fields = w.orderFields(fields) | |
| } | |
| if len(w.FieldsOrder) > 0 { | |
| w.orderFields(fields) | |
| } else { | |
| sort.Strings(fields) | |
| } | |
| 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.
| 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] | |
| }) |
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.