search

reasonml / reason-react

Reason bindings for ReactJS
https://reasonml.github.io/reason-react/
MIT License
3.25k stars 347 forks source link

Add locations-check test #844

Closed jchavarri closed 4 weeks ago

jchavarri commented 2 months ago

I found out today that ppxlib has some -check -locations-check flag that one can use to make sure a ppx generates an AST with valid locations ("valid" in the sense of following the Merlin rules).

I thought this could serve as an alternative to tests like the one @anmonteiro adds in #842 and the many other merlin-based tests that we have.

Rather than building the test based on what merlin returns, we can just make sure the tree is well formed. But it turns out the check doesn't successfully pass (even in 5.1), because there are too many overlaps.

Maybe we can keep the test as is for now and over time solve all issues until we make sure it passed this check. @davesnx @anmonteiro wdyt?

jchavarri commented 4 weeks ago

I wonder if the failing tests will now pass after the ppxlib 0.33 release

They do. I just had to modify one location in the ppx tests (seems not harmful).

jchavarri commented 4 weeks ago

They do. I just had to modify one location in the ppx tests (seems not harmful).

Hm just realized this change was already in https://github.com/reasonml/reason-react/pull/850. I forgot to merge main into this branch 🤦