Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure and run automatic Lua code styling #1931

Merged
merged 13 commits into from
May 30, 2024

Conversation

alerque
Copy link
Member

@alerque alerque commented Dec 13, 2023

I've looked into this before but the available Lua code formatters have always been sub-par and running them on this project has always run into edge cases that create errors and need manual intervention. Hence I've avoided it.

But the more I use Rust with rustfmt and Python with ruff --format and just don't have to think about code formatting the more I like it. I had another look at Lua and there are now two viable options. The one I'm primarily targeting here is stylua, but there is also an attempt to configure the Lua LSP which runs EmmyLuaCodeStyle to format similarly. For now the canonical style will be what StyLua outputs.

I'm not 100% happy with the output and have some upstream issues opened about fixing it, but the fact that we can already run it out of the box and still get a 100% working project back out of it in very little time means I'll probably be headed this way soon.

I'm not in a hurry to merge this to master, but probably will land it in conjunction with merging develop (#1904) prior to releasing v0.15.0.

The good news for existing PRs and future contributors is that because it works 100%, updating existing PRs to match and avoid merge conflicts is one command away.

@alerque alerque added the todo label Dec 13, 2023
@alerque alerque added this to the v0.15.0 milestone Dec 13, 2023
@alerque
Copy link
Member Author

alerque commented Jan 10, 2024

I will probably (but not certainly) be holding off on this for this feature upstream. I expect it to get added eventually, and when it does my strong preference for the space will mean a little bit of churn as everything gets reformatted again. It probably shouldn't be a blocker considering how nice it would be to get 100% deterministic formatting, but yes that's why I'm (probably) holding off on this.

@alerque alerque modified the milestones: v0.15.0, v0.15.1 Jan 10, 2024
@alerque alerque force-pushed the stylua branch 2 times, most recently from 429199b to d3d3bd7 Compare May 29, 2024 13:44
@alerque
Copy link
Member Author

alerque commented May 29, 2024

I'm moving this back to target v0.15.0. The feature I want in stylua is still not upstream, but instead of being a feature request I've now implemented it myself and sent a PR. I don't know if/when it will get included, but having run in on half a dozen projects now including here I'm quite happy with the output.

Given that the current state of affairs is that only a hand formatted file aiming to match our coding style is possible, at worst folks will have to hand format their contributions to match. Best case scenario this comes up in an upstream version soon. Second best scenario folks that really want it automated can already run my fork to format their contributions, or just manually match the style.

We're already at second best:

$ cargo install --git https://github.com/alerque/StyLua --branch space-in-function-defs

I'll probably disable the CI job for now (or at least mark it as not required) since it isn't running the fork (yet).

With all that in mind I plan to move ahead with this. I'm happy to re-format existing PRs or even new ones that fall afoul of the style guide.

@alerque alerque modified the milestones: v0.15.1, v0.15.0 May 29, 2024
@alerque alerque force-pushed the stylua branch 4 times, most recently from f41ab03 to 030bdab Compare May 30, 2024 09:31
@alerque alerque marked this pull request as ready for review May 30, 2024 09:32
@alerque alerque requested a review from a team as a code owner May 30, 2024 09:32
@alerque alerque merged commit a1fd105 into sile-typesetter:master May 30, 2024
14 checks passed
@alerque alerque deleted the stylua branch May 30, 2024 12:51
@Omikhleia
Copy link
Member

So all indentation in all source code files changed from being 2-space based to 3-space based?
(... makes it a bit harder to follow changes on a git blame...)

@alerque
Copy link
Member Author

alerque commented Jun 5, 2024

Yes. We've had a mix for a while, although much more 2× than 3×, and I've been hoping to change it this direction for some time but the tooling to make not just the indentation but all of the formatting automatic just wasn't there until recently.

And yes it does make blame noiser, although you can configure git blame to ignore style change revisions. I've been meaning to add a file that can be used to filter uninteresting but intrusive commits for a while. I'll get one started.

BTW if you have branches or PRs you want to adapt I am quite proficient with the tooling to make it happen now. I am happy to do it for you for anything branched before this change because I know it's ticklish to figure out what to do. The main trick is to rebase and iterate through the commits in reverse chronological order, rewriting one at a time through the branch and applying the formatter. That will clear up all the rebase conflicts without fussing with git rerere and without having to repeat any manual conflict resolution.

@Omikhleia
Copy link
Member

Noted.
So 3 be it !

@alerque
Copy link
Member Author

alerque commented Jun 6, 2024

@Omikhleia A couple of dev tips:

You can now run this in your clone so that all git blame calls ignore a whole bunch of formatting related commits that should make it easier to find actually relevant changes:

$ git config blame.ignoreRevsFile .git-blame-ignore-revs

Also in case you need the magic incantation to restyle all the Lua code in the project, this is what I'm using as the canonical way to format Lua code:

$ cargo install --git https://github.com/alerque/StyLua --branch space-in-function-defs
$ alias stylua="~/.cargo/bin/stylua
$ stylua --respect-ignores -g '*.lua' -g '*.lua.in' -g '*.rockspec.in' .busted .luacov .luacheckrc build-aux/config.ld .

You can of course just run it on a single file, that invocation does everything in the project. You can also do things like run it on save from an editor. It won't do anything to the file at all unless it thinks it can successfully reformat it.

I don't have this feature fork running in CI yet (hoping upstream will release it soonish since the feature itself was approved). When I do get CI going I'll setup a lint for it. You can lint locally with:

$ ./configure STYLUA=~/.cargo/bin/stylua
$ make lint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants