[Format range] Introduce functions to format enclosing range. #295
Conversation
For now we cannot format more granularly than top level forms. Exections functions expected the passed range to cover exactly entire forms. In prospect of user-friendly CLI, we introduce formatting entry points that cover all the top level forms intersecting with passed range.
facebook-github-bot
added
the
CLA Signed
src/erlfmt.erl
Outdated
| @@ -214,6 +218,52 @@ replace_pragma_comment_block(_Prefix, [("%" ++ _) = Head | Tail]) -> | |||
| replace_pragma_comment_block(Prefix, [Head | Tail]) -> | |||
| [Head | replace_pragma_comment_block(Prefix, Tail)]. | |||
|
|
|||
| % This 'enclosing range' variants don't expect you to provide | |||
There was a problem hiding this comment.
I like the comment and maybe this is what we want the behaviour of format_file_range to be and so we can think about moving the comment there, but I don't think we should have two different range formatting functions. I hope that makes sense.
There was a problem hiding this comment.
I agree that both the API and the name of format_file_range are a bit weird. But,
- I'd rather keep some kind of separation (finding the enclosing range VS formatting this range). We could refactor to better split those concerns, but I'd rather do that at the end, when more tests are in place, and scope and design are set.
- I think the entry point (at least for the CLI) should give back the whole file, rather that just the modified part. Next commit is already rearranging those functions. We'll be able to discuss better naming then.
- If the final goal is to get
format_file_rangeto only format the specified range (and not the containing top-level form), it might be clearer to keep the explicitenclosing_rangename until them.
There was a problem hiding this comment.
So it is a bit hard for me to imagine what is to come that will justify having a larger API. I don't think I understand in what context an IDE would support two different modes of formatting in ranges and I believe that is the sole purpose of these range formatting functions. Maybe I am missing something?
There was a problem hiding this comment.
- For IDE, i think it expects only the formatted extract, but I have to double-check that.
- For CLI, it's more convenient for the user to return the whole file, even if only a range was formatted. That also will be internally useful for the 'write in-place' variant.
Then, I prefer small dedicated functions, which might not be part of the public API. By the way I think we should separate public vs internal functions (but needed for unit test) in -export list.
There was a problem hiding this comment.
@michalmuskala Why do we have the possible_ranges function?
My suggestion is that we pick a policy, for example like @slaykachu has done here:
{Starts, Ends} = lists:unzip(PossibleRanges),
Start = lists:min(Starts),
End = lists:max(Ends),
Then format_range always returns {ok, NodesInRange} or {error, Error} and does not need to return {options, PossibleRanges}
What do you guys think?
I am basing this on the suspicion that the Language Server Protocol won't have a need for options and only needs some formatting to happen.
There was a problem hiding this comment.
FYI @pergu answered here: #6 (comment)
That's why we shouldn't answer to questions in the review discussions, but in the code itself!
Also, the 'why' is always useful to add (in comments and in commit description).
There was a problem hiding this comment.
Yes there are multiple options to pick from given a range. I think we should just pick one to start with.
I am happy with {1,2,3} or {2} but open to whatever other option we prefer, but I think we should pick one.
There was a problem hiding this comment.
That's exactly what this 'higher lever' function is doing: picking the biggest range, so that every touched form is eventually formatted.
Should I change the function description to make it clearer? I'm not sure how though! :)
There was a problem hiding this comment.
No I think the description is good.
I think we should replace the implementation of format_file_range with format_file_enclosing_range.
src/erlfmt.erl
Outdated
| Start = lists:min(Starts), | ||
| End = lists:max(Ends), | ||
| Res = format_range(FileName, Start, End, Options, Nodes, Warnings), | ||
| ?assertNotMatch({options, _}, Res), |
There was a problem hiding this comment.
Okay this looks great :D
Final nit: I think we shouldn't import the assert library into our non testing code. I think it is okay to do a manual case match here.
There was a problem hiding this comment.
Oh, ok. What is the rationale? Defensive programming is a thing too, and asserts give a more useful message (also when triggered the the intent is clearer than a mere error).
There was a problem hiding this comment.
My rationale is basically just to stick with current style of programming. I am sure there is a better rationale, but I haven't made the effort to look into it. I hope that makes sense?
[Format range] Introduce functions to format enclosing range.
Context: For now we cannot format more granularly than top level forms.
Existing internal functions expect the passed range to cover exactly entire forms.
In prospect of user-friendly CLI, we introduce formatting entry points
that cover all the top level forms intersecting with passed range.