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

feat: Add std.parseCsv and std.manifestCsv #701

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rohitjangid
Copy link
Member

@rohitjangid rohitjangid commented May 25, 2023

Implement std.parseCsv and std.manifestCsv in standard library

cpp-jsonnet PR: google/jsonnet#1088

@rohitjangid rohitjangid changed the title feat: Add std.parseCav and std.jsonToCsv feat: Add std.parseCsv and std.manifestCsv May 25, 2023
@coveralls
Copy link

coveralls commented May 25, 2023

Coverage Status

coverage: 68.727% (+0.2%) from 68.568% when pulling 4b9c10e on rohitjangid:feat/csv into fed90cd on google:master.

builtins.go Outdated Show resolved Hide resolved
builtins.go Outdated
}

if row == 0 { // consider first row as header
keys = record
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we might make this a user option in future... No need to do it now though.

builtins.go Outdated
for _, k := range record {
keyCount[k]++
if c := keyCount[k]; c > 1 {
keys = append(keys, fmt.Sprintf("%s__%d", k, c-1))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is actually going to be confusing because it's not going to round-trip back through the manifest function (the mangled column names will appear in the output). Also the behaviour would have to be documented which would be a pain. Now you've renamed the function and it doesn't look like a generic CSV reader anymore I think what you were originally doing was fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can add an argument called handle_duplicate_headers=false for users to opt for this. Using this flag, we could either overwrite the duplicate headers or rename the duplicate fields as above. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Made the changes

@rohitjangid
Copy link
Member Author

Hi @sparkprime
Can we merge this if there are no other concerns?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants