-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Lydia Buntrock <[email protected]>
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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? 🏳️⚧️ 🕐 ? 😉
There was a problem hiding this comment.
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.
Also regarding any future PRs: This library has automatic code-formatting. You can cmake the |
Since it's not a super important change for me, but rather a future wish, both options are okay for me. |
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
.