erlfmt
erlfmt copied to clipboard
WhatsApp
add library module for converting between erlfmt and syntax tools
Allows roundtrip conversion from erlfmt:read_nodes() to syntax tools (or erl_parse) and back to erlfmt:format_nodes().
This certainly looks useful for writing refactoring tools in erlang itself.
We are having a hard time deciding if this belongs in erlfmt or a separate library: If we merge it in erlfmt, we would need to maintain it and if we don't merge it, then we would need to document our abstract_forms types better. To merge in erlfmt in would need a barrage of tests, that would make us confident that we can maintain it. Are you willing to do this work?
I only skimmed the code, but I saw a lot of TODOs are they intended to be fixed before merge or ...?
No, the TODOs are meant for doing later, they're not blocking anything. But this code needs to find a home first, and I'm intending to do the work on it. Also hoping to work on reducing the internal differences between erlfmt and syntax tools so the conversion becomes less complicated in places.
Do you also intend to add a barrage of tests, something in the order of https://github.com/WhatsApp/erlfmt/blob/master/test/erlfmt_format_SUITE.erl ?
Knowing this would help us to make a decision.
That would probably be relatively easy to do, just adapting the existing tests and checking that the round-tripped code looks the same, so yes.
That would be one way to get quite a large of coverage, but feel free to take some liberty on what you think is sensible.
Do you also maybe have thoughts on why you think this is the right home for this code?
I think this is the right home, because the erlfmt format doesn't officially exist outside this app. By having the conversion functions here, nobody should need to know about it or depend on it. Also, it becomes easy to make synchronized changes in erlfmt internals and the conversion functions with a single commit.
We have talked and you make a compelling argument :D Lets consider this the future home of this code, but lets also get some tests before merging.
I'm currently pondering what changes I can make in syntax tools that will preserve compatibility with existing code that's using it, but being better at representing the same things under the hood that erlfmt does. Some of those TODOs require that sort of change first. Some other things may be possible to just change on the erlfmt side (after consulting you) and for those I could open tickets if you like.
Is it possible to label the TODOs in the code as syntax_tools TODOs and actual TODOs for this code, to try and make it clear what is do-able by someone only looking at this code? It would at least help me to discern between them.