Fixed bug when other fields also have a "-" instead of an integer.

[fgvieira]

While parsing a VEP stats file from SV calls, got an error:

ValueError: invalid literal for int() with base 10: '-'

It seems it was due to a line in the stats file that had:

Overlapped regulatory features	-

The fix was to always check if the value is - before converting to int.

• This comment contains a description of changes (with reason)
• CHANGELOG.md has been updated

[fgvieira]

Failing because of #1655

[ewels]

The existing code has a try / except block to catch this ValueError already though, so it should already be handled?

Could you attach a file that triggers the error? I guess it must be being raised from somewhere else in the code.

I generally try not to replace - with 0 in MultiQC modules, as they are not quite the same thing. Better to use None or just omit that metric (as is currently done in this module).

[fgvieira]

The try/except block was only when parsing html stats files, not text files.

I can implement the try/except block for text files too, but just felt that it could also catch other unrelated errors. That is why I preferred to handle - specifically. I can also set them to None, if you thing it is better.

[ewels]

Yeah, I think I did it that way because I was handling more than one error at once - see #1597 (both IndexError and ValueError). But yeah, I don't have a strong preference, I agree that specific is probably safer - as long as it also catches the other error mentioned in that issue.

I more meant that I didn't see why that chunk of code needed editing if it was already behaving in the correct way. But you answered my question there - it's the txt parsing code that needed the fix 👍🏻

Please do use None instead of 0. Happy to keep your change from the try/except to the specific case with the - if you like 👍🏻 Hopefully I added the test data from #1597 to the test data repo for the CI checks, but maybe double check with those files just in case if that's ok.

[fgvieira]

Also works with example file from #1597.

[ewels]

Great stuff, thanks! Just tried locally and looks good with one exception. I had thought that passing None would skip it from the table, but it seems that it is printed:

Versus before when it was a mixture of being omitted and showing as 0:

What do you think about adjusting the code to simply not set these keys? Instead of setting them to None. Personally I think that the blank table cells look better (and are just as faithful..?).

Apologies for going back on myself, poor memory of exactly how the MultiQC internals work..

[fgvieira]

Sure, as long as it is consistent. 😄
I feel that having both blank cells and 0 is a bit confusing.

[ewels]

Absolutely! Yes that was surely not intentional.

[ewels]

Many thanks, looking great!