search

Nik-Zainal-Group / signature.tools.lib

R package containing useful functions for mutational signature analysis
Other
80 stars 26 forks source link

fix a small bug in HRDetect.R when using Indels_tab_files #24

Closed ipstone closed 4 years ago

ipstone commented 4 years ago

When running HRDetect_pipeline function using tab indel files, the function still tried to use Indels_vcf_files to identify the indel files. With this fix, it correctly choose from the Indels_tab_files.

pdiakumis commented 4 years ago

@ipstone can you also include readr in the DESCRIPTION file?

ipstone commented 4 years ago

@pdiakumis you are right - that's the other thing I noticed - I am not sure how things work here (meaning how often/likely owner accept pull request), I am happy to do that as well. (though readr is commonly installed with tidyverse). I will amend this pull request soon.

ipstone commented 4 years ago

@pdiakumis updated, including readr in both README and DESCRIPTION

andreadega commented 4 years ago

Hi, thanks for spotting this error!

Previously, I have been happy to merge someone else's contribution. It is a little more complicated than just accepting the request, because our dev branch in not on github, but rather we use github just to publish our stable versions.

I realise now that this was not a good idea. It was nice to have a quick fix added from someone else, but then it introduced this error that you are fixing now, and also it didn't come with the test functions that I usually add whenever I add a new functionality, and so that merge was never properly tested.

So, while I thank you for the fix, I prefer if next time you could raise a bug issue instead, so that I can quickly go and add the fix/enhancement myself and add the appropriate tests before release.

I will work and produce the hotfix today.

Thanks again for your help!

ipstone commented 4 years ago

no problem @andreadega - totally understand your rationale of handing the code base directly. As I went through the code, you did a fantastic job with the tests and clear documentation - it's really high quality work!