sile-typesetter / sile

The SILE Typesetter — Simon’s Improved Layout Engine
https://sile-typesetter.org
MIT License
1.65k stars 98 forks source link

Configure and run automatic Lua code styling #1931

Closed alerque closed 4 months ago

alerque commented 9 months ago

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 commented 8 months ago

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 commented 4 months ago

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.

Omikhleia commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Noted. So 3 be it !

alerque commented 4 months ago

@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