search

jviereck / regjsparser

Parsing the JavaScript's RegExp in JavaScript.
http://www.julianviereck.de/regjsparser/
BSD 2-Clause "Simplified" License
77 stars 20 forks source link

Add test for /a./ #97

Closed nicolo-ribaudo closed 5 years ago

nicolo-ribaudo commented 5 years ago

In https://github.com/mathiasbynens/regexpu-core/pull/29#issuecomment-533523513 I found a bug in regjsgen. I fixed it (https://github.com/bnjmnt4n/regjsgen/pull/13), but when I wanted to add a test I found out that it downloads the test from this reporitory. Even if it's not a bug with your package, I think that a test for a previously untested behavior could be accepted? :stuck_out_tongue:

jviereck commented 5 years ago

Hi @nicolo-ribaudo , thanks for making a contribution to this repo! Sure, happy to add the test (it's not that much in this case).

In case we see a lot more regressions further up in the reg* stack, maybe it would make sense to fix the way the package (in this case the regjsgen) pulls the tests and then apply the new test case in addition to the downloaded ones local to that package. Given that the requested change is small and adding extra infra for having additional extended tests in regjsgen sounds like a big overhead at the current state, I decided it's better to just include the test here.

jviereck commented 5 years ago

Note: Looking at the issue raised in 1 later. Once resolved I will create a new release. If you don't hear back form me within 3 days, please bump me!

nicolo-ribaudo commented 5 years ago

In case we see a lot more regressions further up in the reg* stack, maybe it would make sense to fix the way the package (in this case the regjsgen) pulls the tests and then apply the new test case in addition to the downloaded ones local to that package

:+1: I 100% agree with you

Note: Looking at the issue raised in 1 later. Once resolved I will create a new release. If you don't hear back form me within 3 days, please bump me!

Ok, thanks!

jviereck commented 4 years ago

Created a new release v0.6.1 incorporating this fix and pushed to npm. Sorry for the long delay.