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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TSV file support and a demo #2428

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

weese
Copy link
Member

@weese weese commented Mar 31, 2020

Hey! I needed to modify huge amounts of TSV files and as SeqAn is the one and only library to do this efficiently I wanted to contribute some code 馃槉

@weese weese closed this Apr 2, 2020
@weese weese reopened this Apr 2, 2020
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

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

Hello there! If that is not quite the suprise 馃槃
So first of all thanks for your commitment to add a another file format. We are basically in a development freeze in favour of SeqAn3 but appreciate contributions. But I have some open requests for this. The most important thing is that I don't see any tests, which would be required before this can be added. The second thnig is that it feels more as it could be part of a text_io module which is a generic text file not associated with any particular file formats. Another format could be csv for example and this module would then be structured more like the seq_io.

CTestTestfile.cmake
cmake_install.cmake
Makefile
.vscode
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like a fixup of in-source builds. Can you please remove this but keep the .vscode and make it a separate commit.

Copy link
Member

@marehr marehr Apr 3, 2020

Choose a reason for hiding this comment

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

I think that non of these should be changed, not even .vscode. If you need custom exclusion you can always add those to .git/info/exclude.

Copy link
Contributor

Choose a reason for hiding this comment

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

well true, but .vscode is a common hidden directory for visual studio code which gains popularity. So I wouldn't mind but I have no strong feelings either.

/*!
* @tag FileFormats#Tsv
* @headerfile <seqan/tsv_io.h>
* @brief Variant callinf format file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief Variant callinf format file.
* @brief Variant calling format file.

Why is this bound to variant calling?
As far as I understand it merely reads in tab-separated text files.
Accordingly I would put this all into a text_io module, from which tsv is one supported format. Csv could be another. Similar to seq_io.

/*!
* @class TsvRecord
* @implements FormattedFileHeaderConcept
* @headerfile <seqan/vcf_io.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

copy n paste errors

@eseiler eseiler changed the base branch from develop to main May 13, 2023 14:44
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.

None yet

3 participants