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

[FEATURE] Add filedate to VCF file #30

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Irallia
Copy link
Contributor

@Irallia Irallia commented Mar 2, 2022

This PR is a suggestion to add the filedate to the vcf. I have not yet customized the bcf tests. (I also haven't figured out how yet).
There is no regulation about the format of the filedate, so I decided to use a readable format like ##fileDate=2022-03-02 14:18:22.

Copy link
Member

@h-2 h-2 left a comment

Choose a reason for hiding this comment

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

I like some of this, but I want to have a think about the general direction before progressing with this.

If file_date is important for you, I would propose to make a PR that just adds the member variable (empty by default) and nothing else. We could merge that right away, so you can set the string from your application?

Or we just keep this as a draft for now?

@@ -163,6 +166,7 @@ class header
* \{
*/
std::string file_format = "VCFv4.3"; //!< The file format version.
std::string file_date = transTime(); //!< The file date.
Copy link
Member

Choose a reason for hiding this comment

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

Having file_date as an extra member seems like a good thing! However, I am not sure if I want it to be set automatically, because this results in output not being reproducible by default, which is really annoying when you are checking whether output has changed or not.

Related to this question is also whether BIO should add a line that says that the file was created with BIO. On the one hand, I would like this (advertising for the library); on the other hand, I am often annoyed by bcftools adding so many of those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So another solution would be to add a field which will be set by the user, e.g. the transTime() function would move to iGenVar. Would also be fine for me.

To your related question, I understand the desire to promote the library, but the important question should be whether it helps the user. At first I thought no, because why do I need to know who created it, since I am interested in the results from the tool itself. But then I thought if I don't understand a VCF line or there are errors in it (something like that) I might want to contact BIO instead of the tool creator that just uses the library. Hmm...

*
* \returns a time string in the format: YYYY-MM-DD HH:MM:SS.
*/
std::string transTime()
Copy link
Member

Choose a reason for hiding this comment

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

This function seems a little bit out-of-place here, because it is a generic "make a nice date"-function and not specific

I am also not sure what the name implies? 🏳️‍⚧️ 🕐 ? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that was just a name copy of some funktions I found in the wild wild web.. 🙈 If we ceep the function, we can of course rename it to transformTime for example.

@h-2
Copy link
Member

h-2 commented Mar 3, 2022

Also regarding any future PRs:

This library has automatic code-formatting. You can cmake the tests/format folder and then run make check_library to check if your formatting is correct or make format_library to reformat the files to correct formatting, so you don't have to worry about any style-things.

@Irallia
Copy link
Contributor Author

Irallia commented Mar 7, 2022

I like some of this, but I want to have a think about the general direction before progressing with this.

If file_date is important for you, I would propose to make a PR that just adds the member variable (empty by default) and nothing else. We could merge that right away, so you can set the string from your application?

Or we just keep this as a draft for now?

Since it's not a super important change for me, but rather a future wish, both options are okay for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants