Conversation
hjaensch7
force-pushed
the
hj/csv-builder
branch
from
6451ef9 to
11c3aac
Compare
hjaensch7
force-pushed
the
hj/csv-builder
branch
3 times, most recently
from
07b5182 to
e5ec7d8
Compare
hjaensch7
force-pushed
the
hj/csv-builder
branch
from
620e703 to
4e8dfde
Compare
maloneya
left a comment
maloneya
left a comment
There was a problem hiding this comment.
Feel free to merge, none of my concerns are blocking.
tl;dr
- some suggestions for errors and logging
- some organizational suggestions with our type and schema definitions
- some organizational suggestions for csv utilities
- some style suggestions to improve readability
Additionally i think that writeProjectsCsv is currently super confusing, i don't really understand how the dedupe and leftoverForests things is supposed to work. There is likely a much simpler way of accomplishing this transformation
i would also suggest that we just store the CSVs in Drive not Github
| filePath := "data/pals.csv" | ||
| err := writePALSCsv(config.PalsDataFile, filePath) | ||
| if err != nil { | ||
| log.WithFields(log.Fields{ |
There was a problem hiding this comment.
Worth considering where in the program you want to log your errors.
if by default you log an error every time you if err != nil you end up with pretty polluted log stream
my suggestion would be to have each abstraction decorate the error with its relevant context, i.e.
return fmt.Errorf("Unable to write CSV %w", err)
and allow the top level controller to do the logging
|
|
||
| } | ||
|
|
||
| type PalsEntry struct { |
| } | ||
|
|
||
| func writePALSCsv(original_path string, path string) error { | ||
| // 26 - 43 |
|
|
||
| func writePALSCsv(original_path string, path string) error { | ||
| // 26 - 43 | ||
| purposes := map[string]string{ |
There was a problem hiding this comment.
why are these local variables to the function? perhaps should be a global constant, or also stored in types.go?
| //open original file | ||
| f, err := os.Open(original_path) | ||
| if err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
Nit pick: log.Fatal(fmt.Errorf("writePalstoCSV open call err: %w", err)) would make it easier to pinpoint where in the program the error came from
|
|
||
| // build column headers | ||
| writer := csv.NewWriter(csvFile) | ||
| writer.Write([]string{ |
There was a problem hiding this comment.
any way we can leverage the existing headers for this and just skip the activity?
| // TODO(hank) | ||
| func writeForestCsv(forests []Forest, path string) error { | ||
| // open file | ||
| csvFile, err := os.Create(path) |
There was a problem hiding this comment.
just a general call out that there are multiple places in the code base that deal with the file and csv apis, we could make a single utility that was just
func writeCSV(header []string. rows string[][]) error
and then each transform is just responsible for produce the new rows, but doesn't care about the CSV details
| project.Description, | ||
| project.WebLink, | ||
| project.Location, | ||
| project.Location[10:], |
There was a problem hiding this comment.
??? should probably explain in a comment
| } | ||
|
|
||
| func getDocsAsString(project ProjectUpdate) string { | ||
| allDocs, _ := json.Marshal(project.ProjectDocuments) |
| // dedupe forests first!! | ||
|
|
||
| // remove all but one project update | ||
| forestMap := make(map[int]Forest) | ||
| forestIdsToRemove := []int{} | ||
| // inital map | ||
| for i, forest := range forests { | ||
| // check if forest in map | ||
| if _, ok := forestMap[forest.Id]; !ok { | ||
| forestMap[forest.Id] = forest | ||
| forestIdsToRemove = append(forestIdsToRemove, i) | ||
| } | ||
| } | ||
|
|
||
| // build list of leftover forests TODO clean up | ||
| leftoverForests := []Forest{} | ||
| for i, forest := range forests { | ||
| shouldBeRemoved := false | ||
| for _, index := range forestIdsToRemove { | ||
| if index == i { | ||
| shouldBeRemoved = true | ||
| } | ||
| } | ||
| if !shouldBeRemoved { | ||
| leftoverForests = append(leftoverForests, forest) | ||
| } | ||
| } | ||
|
|
||
| // iterate through leftovers and add states | ||
| for _, forest := range leftoverForests { | ||
| if forestInMap, ok := forestMap[forest.Id]; ok { | ||
| forestInMap.State = forestInMap.State + ", " + forest.State | ||
| forestMap[forest.Id] = forestInMap | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe we make a separate Transform type function that captures all this business logic?
There was a problem hiding this comment.
i would also say that this code is pretty hard to follow
2 participants
Uh oh!
There was an error while loading. Please reload this page.