Pegjs: Feature: Run-time requiring of pegjs files

[brettz9]

Issue type

• Bug Report: no
• Feature Request: yes
• Question: no
• Not an issue: no

Prerequisites

• Can you reproduce the issue?: n/a
• Did you search the repository issues?: yes
• Did you check the forums?: I don't see links to forums
• Did you perform a web search (google, yahoo, etc)?: yes

Description

While there might be other use cases for being able to require PEGJS files directly in Node at run-time, the main intent of this issue is for code coverage (e.g., for Mocha tests).

This latter use case of code coverage would first need #93 (providing source maps) to be first implemented. If implemented, one could avoid the need for a separate instrumented copy of one's tests + peg code.

I see that Node's require.extensions is used to accomplish this for ts-node, and though require.extensions is technically deprecated in Node, as per the discussions around https://stackoverflow.com/questions/28884377/better-way-to-require-extensions-with-node-js (particularly in the vein of this answer), it seems to be the only practical mechanism here, and as per the comment, I don't think we have to worry about Node breaking Babel.

[ExE-Boss]

I’d rather not allow this.

[brettz9]

@ExE-Boss : For our use case, it'd just be for testing (also, not sure if you noted this is the pegjs repo.)

[ExE-Boss]

I know that this is the PEG.js repo, which is exactly why I’m opposed to implementing this here.

[brettz9]

Ok, cool... (I can sometimes follow a link to another repo without noticing it is not a separate issue, so just making sure) :)

[StoneCypher]

No more features until the 2017 two-liners that let us use es6 modules and arrows are out.

[StoneCypher]

Also, it looks like what you actually want is testing

You don't put the coverage hooks in the library. You put them in the test file.

If you want coverage, you can install jest and just be done. It auto-includes everything you need and no modifications need to take place to the library. Coverage tools are designed to be added from the outside, not the inside.

This parser generator should not suddenly become tied to the node.js package ecosystem as an internal feature.

Besides, coverage is a false desire for a parser. Consider the following:

Food
    / "Peanuts"
    / "Shellfish"
    / "Rice"

Now, just write a test for rice. peg is going to try Peanuts first, then when it doesn't work peg will try Shellfish, then peg will try Rice

Notice you only wrote a test for Rice, but you're incorrectly showing coverage for all three

The way a packrat parser (honestly the way ever parser I can think of) works means coverage is fake

Don't waste your time

[brettz9]

As far as putting coverage hooks in the library, normally one puts them in one's tests, yes, but when a parser is auto-generated with some branches as relevant and some as not, then it makes sense for the auto-generator to produce this so that those projects which do not wish to ignore their parser file can do so. It is not related to the coverage of the pegjs library (which has its own coverage requirements) but to coverage of the generated parser's use of the grammar.

In your example, the generated parser has lines of code to check for "Peanuts", "Shellfish" and "Rice", and if they aren't covered, they can indeed be reported.

It becomes more of an open question whether a rule like ruleA = ruleB / ruleC should check that "ruleA" has cases with both "ruleB" and "ruleC", or whether it is sufficient to check that "ruleA", "ruleB", and "ruleC" all get covered somehow (e.g., if there is a ruleD = ruleB / ruleF, its coverage of ruleB may be seen by some projects as enough to avoid the need for checking it is covered within "ruleA", especially if there are lots of alternatives and combinations).

As far as this issue, there could be an entrance file which added the feature, and another which didn't.

[StoneCypher]

@brettz9 - I was about to say "I'm talking to someone else about this same thing" and then I realized it's you, so, I'm going to cease this instance of this discussion in favor of the similar one we're having on #633, if that seems okay to you

I'm writing something long there currently