Fix curate internal quotes take 2 #1565
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 915672e. Per discussion in <#1563 (comment)>, keep CSV-like TSV where quotes may be added or removed, but parsed values should be equivalent.
Resolves <#1312> We are expecting the CSV-like double quoting when there are internal quotes. If the field value is already correctly double quoted, then there should not be any additional quotes.
joverlee521
force-pushed
the
curate-internal-quotes
branch
from
f71d105
to
ad029f6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1565 +/- ##
=======================================
Coverage 70.22% 70.22%
=======================================
Files 76 76
Lines 7987 7987
Branches 1947 1947
=======================================
Hits 5609 5609
Misses 2092 2092
Partials 286 286 ☔ View full report in Codecov by Sentry. |
genehack
approved these changes
Aug 2, 2024
|
Let's discuss this topic more first. I'll open an issue for it. |
|
Saving this here for later… |
tsibley
added a commit
to nextstrain/measles
that referenced
this pull request
Sep 10, 2024
This construction reads a bit clearer and cleaner. It's also a good example of how to use `augur merge`. The limitation on non-seekable streams means the rule now uses additional transient disk space, but it typically shouldn't be an issue. The way Augur's slow start up time impacts `augur merge` also contributes to a longer rule execution time, but it should be negligible in the context of the larger workflow and presumably we'll fix the slow start up eventually.¹ The output is semantically identical but has some syntactic changes re: quoting. It's worth noting that the pre-existing TSV format was _not_ IANA TSV, despite it (still) being treated as such in a few places, but was (and remains) a CSV-like TSV with some quoted fields (and some mangled quotes², e.g. the "institution" column for accession KJ556895). We really need to sort out our TSV formats³, but that's for a larger project. ¹ <nextstrain/augur#1628> ² <nextstrain/augur#1565> ³ <nextstrain/augur#1566>
tsibley
added a commit
to nextstrain/measles
that referenced
this pull request
Sep 10, 2024
This construction reads a bit clearer and cleaner. It's also a good example of how to use `augur merge`. The limitation on non-seekable streams means the workflow now uses additional transient disk space, but it typically shouldn't be an issue. The way Augur's slow start up time impacts `augur merge` also contributes to a longer rule execution time, but it should be negligible in the context of the larger workflow and presumably we'll fix the slow start up eventually.¹ The output is semantically identical but has some syntactic changes re: quoting. It's worth noting that the pre-existing TSV format was _not_ IANA TSV, despite it (still) being treated as such in a few places, but was (and remains) a CSV-like TSV with some quoted fields (and some mangled quotes², e.g. the "institution" column for accession KJ556895). We really need to sort out our TSV formats³, but that's for a larger project. ¹ <nextstrain/augur#1628> ² <nextstrain/augur#1565> ³ <nextstrain/augur#1566>
|
I think this PR should be ready for review based on #1566 (comment). |
tsibley
approved these changes
Oct 3, 2024
tsibley
added a commit
to nextstrain/pathogen-repo-guide
that referenced
this pull request
Oct 3, 2024
This construction reads a bit clearer and cleaner. It's also a good example of how to use `augur merge`. The limitation on non-seekable streams means the workflow now uses additional transient disk space, but it typically shouldn't be an issue. The way Augur's slow start up time impacts `augur merge` also contributes to a longer rule execution time, but it should be negligible in the context of the larger workflow and presumably we'll fix the slow start up eventually.¹ The output is semantically identical but has some syntactic changes re: quoting. It's worth noting that the pre-existing TSV format was _not_ IANA TSV, despite it (still) being treated as such in a few places, but was (and remains) a CSV-like TSV with some quoted fields. We really need to sort out our TSV formats³, but that's for a larger project. ¹ <nextstrain/augur#1628> ² <nextstrain/augur#1565> ³ <nextstrain/augur#1566> Ported-from: <nextstrain/measles@4d73b7f> Related-to: <nextstrain/measles#52> Related-to: <#65>
3 tasks
genehack
approved these changes
Oct 7, 2024
| lineterminator='\n', | ||
| quoting=csv.QUOTE_NONE, | ||
| quotechar=None, | ||
| lineterminator='\n' |
There was a problem hiding this comment.
very nit, much pick
Suggested change
| lineterminator='\n' | |
| lineterminator='\n', |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of proposed changes
Follow the general pattern of creating CSV-like TSVs as discussed in #1563 (comment).
We are expecting the CSV-like double quoting when there are internal quotes. If the field value is already correctly double quoted, then there should not be any additional quotes.
Related issue(s)
Resolves #1312
Checklist