Add option for formatting with spaces between function names and arguments #839
Conversation
alerque
force-pushed
the
space-in-function-defs
branch
from
94ba46a
to
530aa62
Compare
alerque
force-pushed
the
space-in-function-defs
branch
from
530aa62
to
cb823a1
Compare
|
I'm using this in production including setting the config option it introduces all over the place. The results are what I want to see. If the implementation here isn't the best way to do it or needs adjusting to be included I'm happy to adapt, but it would be nice if this could get upstreamed so I don't have to push by fork of StyLua on anybody contributing to my Lua projects ;-) |
|
I think I'm going to get some movement on #854 this weekend (which I want to merge before next release) so I also hope to review this too. Sorry for the wait! |
|
Sure fair enough. From an implementation standpoint there are probably better ways to do this (I was quite unfamiliar with the code base) even with the current parser, and improved full-moon interfaces may open up even better ways. From the perspective of the end user feature introduction the two important parts are the name of the new config option key and values paired with their respective expected test outputs. Everything in between is implementation details that could be refactored at any time. Personally I'm only interested in the |
There was a problem hiding this comment.
Implementation itself looks good to me, thank you!
the name of the new config option key and values paired with their respective expected test outputs
I'm trying to think if the key name is complete descriptive of what is actually happening here for someone with no context. The space isn't really after the function, so maybe it should be space_in_functions or space_in_functions_before_parentheses. Not sure though, naming is always hard.
| @@ -1158,7 1165,8 @@ pub fn format_function_declaration( | |||
| shape | |||
| ) | |||
| .update_leading_trivia(FormatTriviaType::Append(leading_trivia)); | |||
| let formatted_function_name = format_function_name(ctx, function_declaration.name(), shape); | |||
| let formatted_function_name = format_function_name(ctx, function_declaration.name(), shape) | |||
| .update_trailing_trivia(FormatTriviaType::Append(function_definition_trivia)); | |||
|
|
|||
| let shape = shape (9 strip_trivia(&formatted_function_name).to_string().len()); // 9 = "function " | |||
There was a problem hiding this comment.
A point worth mentioning is that our "shape" calculation here (and potentially elsewhere) will not take into account the fact that a space was added after the function name (since we explicitly strip the trivia afterwards). Shape is used to store the amount of line budget used so far and how many characters we have left on a line. This opens up a very small edge case where the definition line now hits column_width 1 characters because of the space, but shape only sees column_width and therefore we don't reformat correctly. It's quite a minor edge case though, unsure if it is worth rectifying until someone brings it up
There was a problem hiding this comment.
I'd be happy to fix the edge case if I had any idea where/how, but I wasn't actually that clear on the internals of how that was being done or where extra info should be passed to get it back. Given what you say here I'm guessing the same edge case might exist for some other scenarios unrelated to this PR, no?
alerque
force-pushed
the
space-in-function-defs
branch
from
cb823a1
to
1cb1676
Compare
Yes naming is hard. I'm happy to refactor this to another name if you think it best, but I'm not convinced there are better options. Actually instead of I don't like |
|
The lint error seems to be unrelated to this PR and more related to a new Rust version. In fact it might even be a false positive for the new lint, I know there was some talk of false positives in new docstring lints. I haven't looked into whether this is one of them, but it seems that should be in a separate PR anywhere. |
alerque
force-pushed
the
space-in-function-defs
branch
from
1cb1676
to
6cef63c
Compare
|
The force-push just now was to add a missed bit to override the configs if CLI arguments were present. The option was working from config (or editor config) and accepted from the CLI but silently ignored. |
Closes #832
So far just some CLI options and tests. Work on implementation pending feedback in issue.All options implemented and tested.