bcoe / c8

output coverage reports using Node.js' built in coverage
ISC License
2k stars 91 forks source link

feat: c8 ignore else #231

Open mikeal opened 4 years ago

mikeal commented 4 years ago

The original issue logged for ignore rules asked for “all of them” but only c8 ignore next was added. I didn’t think that was much of a big deal until I realized it takes 3 ignore next statements to ignore an else clause with only one line. That’s actually pretty annoying, it would really help to get this in.

bcoe commented 4 years ago

@mikeal could you use /* c8 ignore next 3 */ and give it a count?

Are there any better constructs we could give, without actually parsing the AST.

mikeal commented 4 years ago

Maybe. The thing is, I’m not sure how much of this is really just a bug. Putting an ignore statement after the throw for instance, that just doesn’t make sense.

But given that the number (3) doesn’t correspond to the number of lines, it makes it a difficult and error prone approach. Most programmers aren’t going to know that 3 isn’t a line count and edits to that code may reduce the needed ignore number, which they won’t notice and later coverage may be ignored.

At some point I think you’re gonna have to parse the AST because that’s the only way you’ll be able to present developers with ignore capabilities that match the mental model they have for their own code 🤷‍♂️

Also, isn’t the AST already parsed? Can’t we get the AST from V8?

bcoe commented 4 years ago

Also, isn’t the AST already parsed? Can’t we get the AST from V8?

I don't believe it is currently exposed through Node.js; this was a suggestion in our Node tooling meeting at OpenJS world, and I liked it ... however ...

The issue is, we don't just need an AST, we need an AST for whatever flavor of JavaScript we're showing in the report:

Unfortunately, JavaScript is a confluence of languages at this point...

Having said this, if we were could get an actual AST from v8 through Node.js, it seems like we could allow folks to use it with a flag? and support slightly smarter ignore rules.