-
Notifications
You must be signed in to change notification settings - Fork 33
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
ToConfig with columns order for CSV #47
Conversation
pgrandinetti
commented
Jun 19, 2024
- Added new ToConfig to force order of columns in CSV.
- Added tests for new ToConfig.
- Added tests for new ToConfig.
Hi. I know QFrame doesn't hold order of columns, but I needed it in the CSV. Didn't find a way to do it, so I implemented it.
|
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.
Great, thanks!
Seems functionally correct. There are a few nitpicks on style that you may want to consider.
qframe.go
Outdated
if len(conf.Columns) != len(qf.columns) { | ||
return qerrors.New("ToCSV", fmt.Sprintf("wrong number of columns: expected: %d", len(qf.columns))) | ||
} | ||
iterCols = make([]namedColumn, 0, len(qf.columns)) |
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.
Just some nitpicking and suggestions for how to make the code a bit shorter and more idiomatic:
iterCols = make([]namedColumn, len(qf.columns)) // Pre-set length
for i := range conf.Columns { // Use range expression
cName := conf.Columns[i] // camelCase name
if col, ok := qf.columnsByName[cName]; !ok {
return qerrors.New("ToCSV", fmt.Sprintf("%s: column does not exist in QFrame", cName))
}
// Skip the else-clause since there is a return in the if-clause.
// Set element using indexing rather than append (since we now have a length set)
iterCols[i] = col
}
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, thank you @tobgu
Made all recommended changes, except the last one: when col
is defined inline in the if
, then it needs a else
branch to be used, or the compiler will reject the code. See minimal example here: https://go.dev/play/p/2bowv-oHDl5
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.
LGTM!