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

add special cases for csv and tsv formatting [IMPLY-34859] #294

Conversation

mcbrewster
Copy link
Contributor

We want null columns to be represented by a space and empty strings to be represented by "". To support this we need to add in special logic in both the TSV and CSV finalizers to return the right values.
Screen Shot 2023-07-04 at 3 44 32 PM
Screen Shot 2023-07-04 at 3 44 04 PM

@mcbrewster mcbrewster requested a review from jgoz July 4, 2023 22:44
@mcbrewster mcbrewster changed the title add special cases for csv and tsv formatting add special cases for csv and tsv formatting [IMPLY-34859] Jul 4, 2023
@jgoz
Copy link
Contributor

jgoz commented Jul 4, 2023

We want null columns to be represented by a space and empty strings to be represented by "".

Do we want this in both CSV and TSV? Because it looks like empty string handling only applies to TSV

@mcbrewster
Copy link
Contributor Author

mcbrewster commented Jul 4, 2023

We want null columns to be represented by a space and empty strings to be represented by "".

Do we want this in both CSV and TSV? Because it looks like empty string handling only applies to TSV

It has a check for v !== "", if it is an empty string then it will use the string escaping logic that csv's use and that will return '""'. I was just trying to reuse what was there but I can make it more explicit.

@jgoz
Copy link
Contributor

jgoz commented Jul 5, 2023

Maybe I'm being dense, but I don't see where the empty string will get transformed for CSVs. I see where quotes get escaped, but empty string won't contain quotes...

@mcbrewster
Copy link
Contributor Author

mcbrewster commented Jul 5, 2023

Maybe I'm being dense, but I don't see where the empty string will get transformed for CSVs. I see where quotes get escaped, but empty string won't contain quotes...

if it's if **(v !== ''** && v.indexOf('"') === -1 && v.indexOf(',') === -1) return v;
therefore empty string will drop down and return:
"${v.replace(/"/g, '""')}"
you can see it is a template string with quotes and then the inside gets escaped. I am just going to change it though to make it more readable.

Copy link
Contributor

@jgoz jgoz left a comment

Choose a reason for hiding this comment

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

🍣 Thank you. I was definitely being dense and missed the quotes that were wrapping the fallthrough case. But I like the explicit version where TSV and CSV are more similar to each other.

@jgoz jgoz merged commit db85e1d into master Jul 5, 2023
@jgoz jgoz deleted the IMPLY-34859-downloads-should-use-two-consecutive-commas-for-null-downloads branch July 5, 2023 16:07
@mcbrewster mcbrewster mentioned this pull request Jul 5, 2023
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.

2 participants