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

Stop quoting TSV outputs from augur curate #1493

Merged
merged 4 commits into from
Jun 27, 2024
Merged

Conversation

joverlee521
Copy link
Contributor

@joverlee521 joverlee521 commented Jun 27, 2024

Description of proposed changes

Resolves #1312 by never quoting output fields from write_records_to_tsv.
See commits for details.

Checklist

  • Checks pass
  • If making user-facing changes, add a message in CHANGES.md summarizing the changes in this PR

@joverlee521 joverlee521 requested a review from a team June 27, 2024 00:14
Shows the undesired behavior with internal quotes described in
<#1312> comes from the
`write_records_to_tsv` function.
According to TSV specs,¹ there are no restrictions on special characters
other than tabs are not allowed in a field. This is different from the
CSV specs,² which require double quotes around fields that contain
special characters.

Since this function only produces TSVs, follow the TSV specs and stop
adding quotes.

Resolves <#1312>

¹ <https://www.iana.org/assignments/media-types/text/tab-separated-values>
² <https://datatracker.ietf.org/doc/html/rfc4180#page-2>
@joverlee521
Copy link
Contributor Author

Rebased on to master to prevent merge conflicts in the changelog and added a changelog entry.

@joverlee521 joverlee521 merged commit 0adfe3d into master Jun 27, 2024
20 checks passed
@joverlee521 joverlee521 deleted the curate-internal-quotes branch June 27, 2024 20:56
@victorlin
Copy link
Member

@genehack how did you approve a merged PR?? I haven't seen that option so I use a ✅ comment instead...

@joverlee521
Copy link
Contributor Author

@genehack how did you approve a merged PR?? I haven't seen that option so I use a ✅ comment instead...

Huh, gh cli allows it: cli/cli#5038

(I just tried it on #1500)

@genehack
Copy link
Contributor

genehack commented Jul 1, 2024

@genehack how did you approve a merged PR?? I haven't seen that option so I use a ✅ comment instead...

I had it open in a tab from friday and didn't realize it had been merged when I approved it — the tab I was looking at said "OPEN" still!

@victorlin
Copy link
Member

Huh, gh cli allows it: cli/cli#5038

Oh neat! Maybe I'll use that in the future... or just stick to the emoji if I'm lazy

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.15%. Comparing base (dc97cde) to head (23b80e5).
Report is 264 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1493   +/-   ##
=======================================
  Coverage   69.15%   69.15%           
=======================================
  Files          70       70           
  Lines        7694     7694           
  Branches     1887     1887           
=======================================
  Hits         5321     5321           
  Misses       2087     2087           
  Partials      286      286           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

augur curate passthru can add double quotations
3 participants