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

Commas in Sample IDs break the internal manifests for per-sample fastq formats #157

Open
thermokarst opened this issue Nov 9, 2017 · 4 comments
Labels
type:bug Something is wrong.

Comments

@thermokarst
Copy link
Contributor

thermokarst commented Nov 9, 2017

References
This recently came up on the forum.

@thermokarst thermokarst added the type:bug Something is wrong. label Nov 9, 2017
@jairideout
Copy link
Member

I'm not sure we can do much about that since the format is CSV. Users will need to quote any fields containing commas to "escape" the delimiter. Do you think describing this edge-case in the "fastq manifest" part of the importing tutorial would resolve the issue?

@thermokarst
Copy link
Contributor Author

Couldn't we start emitting quoted fields and/or escaped fields in the internal manifest? It looks like we are already using pandas.read_csv, to read those manifests back into Q2, which should be able to handle quoted vs non-quoted editions of these files. I think we could do that in a backwards-compatible way, but maybe I am missing something! One gotcha is that there are a few places in the ecosystem where we grab the manifest and do some things with it, so depending on how that is currently implemented, there could be some unintended consequences 👿 .

I am up for documenting the edge case if we don't have a quick fix for this!

@jairideout
Copy link
Member

👍 for updating the places where qiime2 is reading/writing manifests so they're compatible with commas. There'll still be the opportunity for users to run into the issue when they're writing their own manifest file (which they'll need to quote themselves), so some docs on the importing tutorial may be useful in addition to the q2-types updates. Does that sound good to you?

@thermokarst
Copy link
Contributor Author

Sounds good to me (updating q2-types and documenting the edge case)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something is wrong.
Projects
None yet
Development

No branches or pull requests

2 participants