Skip to content

handle \s+ in VCF - #782

Merged
Peter-J-Freeman merged 3 commits into
developfrom
vcf_cnv
Feb 16, 2026
Merged

handle \s+ in VCF#782
Peter-J-Freeman merged 3 commits into
developfrom
vcf_cnv

Conversation

@Peter-J-Freeman

Copy link
Copy Markdown
Collaborator

No description provided.

@John-F-Wagstaff John-F-Wagstaff left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, a few comments but only 1 fix, which could be put off for later if needed.

Firstly: If we wanted to be more strict on inputs to-be processed you could trigger a strip on the input before testing, in vvMixinCore, but this would only eliminate broken inputs that will drop out with a error about column numbers anyway so this is fine as is.

Secondly: Testing for "\t" first then regex for '\s+' will be faster for tabs but slightly slower on the non tab containing inputs. As non-tab inputs are non-standard, and also should be less common, this looks fine to me.

Finaly: We end up not splitting on ',' in the split_vcf_line function, after this patch, given that we skip comma deliminated inputs already this also checks out, However the VcfConversionError on line 21 might need updating here though, as the text no longer matches the code.

So you might want to get in a fix on the VcfConversionError while we are here.

@Peter-J-Freeman

Copy link
Copy Markdown
Collaborator Author

@John-F-Wagstaff, this should then deal with these useful thoughts. A little additional code, but likely faster for correct \t versions

@Peter-J-Freeman

Copy link
Copy Markdown
Collaborator Author

note, I wanted CSV, but comma is used so much elsewhere it would be a total nightmare

@Peter-J-Freeman
Peter-J-Freeman merged commit 1bafc7c into develop Feb 16, 2026
1 check passed
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.

2 participants