StyLua icon indicating copy to clipboard operation
StyLua copied to clipboard

Add option for formatting with spaces between function names and arguments

Open alerque opened this issue 2 years ago • 7 comments

Closes #832

~~So far just some CLI options and tests. Work on implementation pending feedback in issue.~~ All options implemented and tested.

alerque avatar Dec 23 '23 14:12 alerque

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 ;-)

alerque avatar Jun 28 '24 19:06 alerque

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!

JohnnyMorganz avatar Jun 28 '24 20:06 JohnnyMorganz

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 SpaceAfterFunctions:::Definitions option here, but besides Always and Never I also went ahead and implemented the Calls variant because I've seen code bases that use that. The default is of course Never so as not to cause churn in existing users.

alerque avatar Jun 28 '24 20:06 alerque

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.

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 space_after_functions the more correct name might be space_after_function_names. It just seems wordy.

I don't like space_in_functions or space_in_function_before_parenthesis because both suggest a different location entirely in my mind.

alerque avatar Aug 11 '24 11:08 alerque

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 avatar Aug 11 '24 11:08 alerque

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.

alerque avatar Aug 15 '24 13:08 alerque