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

docs: reformat lua code #2273

Closed
wants to merge 1 commit into from
Closed

docs: reformat lua code #2273

wants to merge 1 commit into from

Conversation

azzamsa
Copy link
Contributor

@azzamsa azzamsa commented Jul 17, 2022

I build an app called Gelatyx for this purpose. It worked well. However, StyLua find some invalid code such;

�[31merror parsing: error occurred while creating ast: unexpected token `(`. (starting from line 1, character 9 and ending on line 1, character 10)
additional information: expected function name�[0m
�[31merror parsing: error occurred while creating ast: unexpected token `(`. (starting from line 3, character 9 and ending on line 3, character 10)
additional information: expected function name�[0m
�[32mdocs/config/lua/ExecDomain.md�[0m is formatted

It says the code is missing function name:

Here is the full list of files that has invalid code:
result.txt

Usage;

$ gelatyx lua --file docs/**/*.md --check

fixes: #2253

@wez
Copy link
Owner

wez commented Jul 17, 2022

Thanks for this!

Seeing what it changes, I hope we can configure some of the opinions of sylua.

A number of the choices in the docs and the set of modules and types in wezterm were carefully selected to work with lua's syntax to make things reasonably expressive without excessive punctuation.

The things that stand out to me as undesirable in the styling on my first pass through are:

  • Indent depth is different than my preference; for example docs/config/lua/config/foreground_text_hsb.md in this PR should ideally only add the trailing , on the closing } and leave the whitespace alone
  • local wezterm = require 'wezterm' vs local wezterm = require("wezterm"). Not a deal breaker, but the parens are not needed and less punctuation makes the file feel cleaner and less complexer.
  • local new_pane = pane:split{} vs local new_pane = pane:split({}) (the latter is awful IMO)

Aside from controlling the styling, for this to stick I think we'd need to fold this tool into the doc build and CI:

  • Teach ci/build-docs.sh to run gelatyx
  • Augment CI pages workflow to install gelatyx, and then verify that git status is clean after running it to encourage keeping the docs correctly formatted.

@azzamsa
Copy link
Contributor Author

azzamsa commented Jul 18, 2022

I reformated the docs using Gelatyx v0.1.4 with this command and configuration.

$ gelatyx lua --file docs/**/*.md --language-config config.toml

config.toml

column_width = 120
line_endings = "Unix"
indent_type = "Spaces"
indent_width = 2
quote_style = "AutoPreferDouble"
call_parentheses = "NoSingleString"

Config reference: https://github.com/JohnnyMorganz/StyLua#options

return {
font = wezterm.font("JetBrains Mono"),
font = wezterm.font "JetBrains Mono",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the risk of sounding mercurial, this is one case where I would lean towards having parens

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like no. I think I'd rather this didn't have parens if it means that is what is needed for the pane:split and wezterm.action stuff to not have parens

{
key = "a",
mods = "LEADER",
action = act.ActivateKeyTable({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and these act cases don't need parens. What are the chances that we can control the formatting to be this specific?

@@ -16,7 +16,7 @@ Splits `pane` and spawns a program into the split, returning the
`MuxPane` object associated with it:

```lua
local new_pane = pane:split{}
local new_pane = pane:split({})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it is a stylua bug; this seems like the no-single-parens config should have applied?

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this looks better, but it's still not quite how I like.

Can we try:

call_parentheses = "None" and column_width = 78?

@wez
Copy link
Owner

wez commented Jul 18, 2022

Can we also set quote_style = "AutoPreferSingle"

@azzamsa
Copy link
Contributor Author

azzamsa commented Jul 18, 2022

Should I override the current commit with new config, or is it better to create a new commit?

@wez
Copy link
Owner

wez commented Jul 18, 2022

I think it's fine to force-push here so we can see the diff.

In terms of merging this, I was thinking that I don't trust the GH UI to let me see everything in a large diff, and what I'd like to see is a new separate PR that adds the style config and call to gelatyx in ci/build-docs.sh so that I can run and inspect locally before committing the actual reformatted docs. That would mean updating .github/workflows/pages.yml to install gelatyx and stylua in order for the CI to continue to work.

@azzamsa
Copy link
Contributor Author

azzamsa commented Jul 19, 2022

I have updated the ci and added the config.

wez pushed a commit that referenced this pull request Jul 19, 2022
wez added a commit that referenced this pull request Jul 19, 2022
Don't format generated docs; their inputs should be formatted instead.

refs: #2273
refs: #2253
wez added a commit that referenced this pull request Jul 19, 2022
wez added a commit that referenced this pull request Jul 19, 2022
@wez
Copy link
Owner

wez commented Jul 19, 2022

I split the docs changes out from your change to the build scripts, then did some work to address the errors in the doc pages; that was a bit frustrating because the error output is lacking in context, and formatting seems non-deterministic. I filed two issues for that:

I've pushed the end result of the formatting in e0ea0f4

Thanks for working on this!

@wez wez closed this Jul 19, 2022
@azzamsa
Copy link
Contributor Author

azzamsa commented Jul 28, 2022

Thank you so much for merging this PR. Very glad to be part of wezterm contributor.

I will keep maintaining Gelatyx. Yes, I have replied to both of your issue.

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

Successfully merging this pull request may close these issues.

Consistent Lua code formatting with StyLua
2 participants