[Format range] Add --range as a command line option. #299
Conversation
This commit plugs the CLI option to existing range formatting function.
facebook-github-bot
added
the
CLA Signed
src/erlfmt_cli.erl
Outdated
| }. | ||
|
|
||
| resolve_width(undefined, W) -> W; | ||
| resolve_width(W, _) -> W. | ||
| resolve_undefined(undefined, W) -> W; |
There was a problem hiding this comment.
A nit, take it or leave it
But I preferred having verbose functions for each parameter
There was a problem hiding this comment.
Argl, it would trigger my DRY obsession!
There was a problem hiding this comment.
No problem, but just to explain where my review is coming from.
I prefer readability over DRY.
DRY is only important to me if it removes a large maintenance burden.
There was a problem hiding this comment.
Having two functions doing the same thing doesn't match my definition of readability :)
'Same things should share a same name, different things should have distinct names'
Yet, I don't have strong feelings for this one, so I've switched back to one function per parameter.
src/erlfmt.erl
Outdated
| @@ -234,7 +262,7 @@ format_file_range(FileName, StartLocation, EndLocation, Options) -> | |||
| string(), | |||
| erlfmt_scan:location(), | |||
| erlfmt_scan:location(), | |||
| [{print_width, pos_integer()}] | |||
| config() | |||
There was a problem hiding this comment.
The options verbose and pragma are not supported by this function, that is why this is a smaller property list.
Could we change this back?
There was a problem hiding this comment.
So, the range was extracted for dispatching purpose, but still sits in this properly list. Would I rather:
- Remove it from the properly list? Con: that's additional work just to
- Introduce a sub_config() type with just
print_widthandrange? - Other. Con: I have no idea what this alternative is.
There was a problem hiding this comment.
We want the function spec to only support what the function actually can support and not say that it can actually influence verbosity or where the output goes.
No description provided.