fix(grainfmt): Refactor code formatter #1042
Conversation
| let single_ast_line = "x" | ||
| let single_ast_line = "x" |
There was a problem hiding this comment.
Why did this change if it uses // formatter-ignore?
There was a problem hiding this comment.
Keen eye! There was a good reason for it to do with locations, I'll have another look though
There was a problem hiding this comment.
Oh, because it's really hard (impossible) to add the spacing back. We don't know how much padding the formatter is adding around where we are inserting. So you can see it works in the line above but not here. I can't see how to fix it presently.
There was a problem hiding this comment.
I figured this was the best of a poor choice
There was a problem hiding this comment.
@marcusroberts This was an absolute delight to read through. Awesome work! I had a few more pieces of advice in reformat.re
compiler/grainformat/reformat.re
Outdated
| switch (comments) { | ||
| | [] => separator | ||
| | _ => | ||
| let last_comment = List.nth(comments, List.length(comments) - 1); |
There was a problem hiding this comment.
Just musing: I wonder what the performance characteristics of your impl instead of List.hd(List.rev(comment)) (because you have to iterate the whole list to get the length and then you have to iterate it again to get the nth item?)
There was a problem hiding this comment.
A good question, and I wondered the same. The number of comments in the list should be short, as this is usually scoped to a statement , expression etc. Worth changing do you think?
There was a problem hiding this comment.
I want to defer to @ospencer because maybe he has more understanding of the implications of each.
There was a problem hiding this comment.
This is probably much faster since it avoids all the allocations of creating a new list if you do List.rev. But of course, the fastest would be a recursive function that just pulls the last item 🙂
I've rebuilt the formatter to use a context aware approach to determining comments to print. This saves the pre-scan of locations and comments and has much better performance.
I've also heavily refactored the code to really improve maintainability and now brings consistent formatting to anything that is iterated (e.g. top level statements, block statements, record fields etc). There are some improvements like supporting start of line comments too.