google / vim-coverage

Apache License 2.0
101 stars 22 forks source link

coverage#CreateReport: move assertions up #23

Closed blueyed closed 6 years ago

blueyed commented 6 years ago

This makes it easier in case of errors to see where it is coming from.

dbarnett commented 6 years ago

Can you give more context, like what the error looks like w/ and w/o? We use this convention widely across all our plugins, and it'd be unfortunate if the errors aren't precise, but also I'm hesitant to start unwinding this kind of thing everywhere it appears and I don't see any special reason we'd do it here and not everywhere else.

blueyed commented 6 years ago

Currently I am seeing:

Error rendering coverage: ERROR(WrongType): Expected a list. Got a null.

(using https://github.com/google/vim-maktaba/pull/213 to fix the type name)

The error will be the same, but it's easier to figure out where it's coming, since you can easily comment the lines. Not very helpful altogether, but I guess that's where this PR comes from.

When throwing a custom exception you see that it is coming via function coverage#Show[5]..<SNR>110_CoverageShow[9]..coverage#ShowCoverage[45]..15[21]..coverage#CreateReport.

Therefore additionally/instead v:throwpoint should get added to caught exceptions to aid debugging in case of errors. With v:throwpoint and this PR you will have the correct error line (since previously it's a single line).

blueyed commented 6 years ago

The error/issue I was seeing: https://github.com/google/vim-coverage/pull/30

dbarnett commented 6 years ago

Sounds like you're saying the issue is that there are 3 ensures in the same statement. We should probably change it around to give a more useful error if it's important that the errors are useful. I guess you can proceed w/ splitting these out if it helps.

blueyed commented 6 years ago

Yup, thanks!

dbarnett commented 6 years ago

(I approved. You have a button to merge the changes now, right?)

blueyed commented 6 years ago

@dbarnett

You have a button to merge the changes now, right?

No. Did I miss an invite?

dbarnett commented 6 years ago

I approved them so I thought you could. Anyway, I merged them for you.