Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 3 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: 55.826% (+0.2%) from 55.582%
when pulling 39b945f on rohitjangid:feat/csv
into 2f73f61 on google:master.

builtins.go Outdated
}

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

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
Collaborator

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
Collaborator

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?

@rohitjangid
Copy link
Member Author

@sparkprime nudging this PR

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

Successfully merging this pull request may close these issues.

3 participants