Add formatter for .talon files to pre-commit #947

wenkokke · 2022-08-01T18:39:31Z

This PR adds talonfmt, as well a tabs-to-spaces conversion, to pre-commit.

The changes in the .talon should absolutely be reviewed.

Note that talonfmt supports alignment of context headers and one-line commands, but I've not enabled those options.

rntz · 2022-08-01T19:24:42Z

This appears to remove all empty lines from .talon files, which I'm strongly against - it should be possible to separate commands into groups by using empty lines.

It also appears to make all single-line commands into multi-line-with-indent, which I also dislike. I'd be okay with doing this for long lines (and/or collapsing commands into a single line when it would be short enough).

wenkokke · 2022-08-01T19:32:45Z

It also appears to make all single-line commands into multi-line-with-indent, which I also dislike. I'd be okay with doing this for long lines (and/or collapsing commands into a single line when it would be short enough).

That’s just the default. I've set a max line width of 80 so talonfmt doesn't just pick whatever gives the shortest line widths.

wenkokke · 2022-08-01T19:33:28Z

This appears to remove all empty lines from .talon files, which I'm strongly against - it should be possible to separate commands into groups by using empty lines.

What do you feel should happen with blank lines?

pokey · 2022-08-02T06:04:52Z

What do you feel should happen with blank lines?

I would argue that any sequence of blank lines should be collapsed into a single blank line. And any whitespace on the blank line should be removed

pokey · 2022-08-02T06:06:46Z

Super minor, but it would be nice if comments had a space after the #. Prob not worth fixing if that's going to take much time tho

rntz

I'd prefer if empty lines were simply left alone (with whitespace removed), but combining adjacent empty lines would also be ok.

I'm against changing comments by adding a leading space, though. Comments are for expressing human intent so I think that autoformatters should leave them alone.

The PR looks much better now that it tries to put things on a single line where possible. It is removing the nice horizontal alignment some .talon files have, though. I remember there was some discussion on the corresponding issue of the right way to do this to avoid large diffs - how does talonfmt handle alignment?

apps/i3wm/i3wm.talon

apps/generic_debugger.talon

pokey · 2022-08-02T08:17:12Z

I'd prefer if empty lines were simply left alone (with whitespace removed), but combining adjacent empty lines would also be ok.

I think it's pretty common for autoformatters to collapse adjacent empty lines, unless the spec calls for them (eg before a function in Python)

One place I'd be tempted to remove empty lines is before / after the - context separator line. I think the autoformatter should have an opinion there.

I would also be tempted to remove empty lines if they appear after the rule but before the body of a command

I'm against changing comments by adding a leading space, though. Comments are for expressing human intent so I think that autoformatters should leave them alone.

I guess I don't feel too strongly here. I'm pretty sure both other autoformatters that we use do this, though. And I'm not sure I see what intent one would be expressing by not including a space between the # and the start of the comment.

But again, I don't feel strongly, and it's easier to leave them untouched

wenkokke · 2022-08-02T16:48:19Z

What do you feel should happen with blank lines?

I would argue that any sequence of blank lines should be collapsed into a single blank line. And any whitespace on the blank line should be removed

@pokey @rntz What about "1 blank line is left alone, 2 or more blank lines become 2 blank lines" at the top level, but all blank lines in the context header, after the "-", and at the start of a command body are removed? That way you can group commands but there's still some consistency.

wenkokke · 2022-08-02T18:22:07Z

I'm against changing comments by adding a leading space, though. Comments are for expressing human intent so I think that autoformatters should leave them alone.

@rntz I think that while all code formatting expresses human intent, the purpose of a formatter is to enforce some measure of consistency. However, I've added a "--no-format-comments" flag, so we can at least not hardcode this behaviour.

wenkokke · 2022-08-02T18:27:25Z

I guess I don't feel too strongly here. I'm pretty sure both other autoformatters that we use do this, though. And I'm not sure I see what intent one would be expressing by not including a space between the # and the start of the comment.

But again, I don't feel strongly, and it's easier to leave them untouched

I think the best option would be to use, e.g., comment_formatter on blocks of comments. Outsource it, but ideally find something that respects indented blocks.

wenkokke · 2022-08-02T22:53:25Z

Changes:

Set --no-format-comments to preserve comments as-is.
Added logic to keep either 1 or 2 blank lines intact, and condense 3 or more blank lines down to 2.

phillco · 2022-08-02T23:26:37Z

This looks great. I find the horizontal alignment inside of existing Talon files to be a bit distracting, so I'm fan of the way they currently look in the PR.

wenkokke · 2022-08-02T23:34:02Z

This looks great. I find the horizontal alignment inside of existing Talon files to be a bit distracting, so I'm fan of the way they currently look in the PR.

@phillco do you have any ideas how to resolve the issues with pre-commit.ci? It currently doesn't have C++ build tools, which breaks tree_sitter.

phillco · 2022-08-02T23:40:05Z

I'm not sure. If they don't support it out of the box I'm guessing we might have to move to Github Actions.

wenkokke · 2022-08-02T23:48:56Z

I'm not sure. If they don't support it out of the box I'm guessing we might have to move to Github Actions.

Works locally, it's just not working with the CI.

pokey · 2022-08-03T05:49:32Z

Maybe you could have a branch of talonfmt where you check in the compiled code so that it doesn't need to compile?

wenkokke · 2022-08-03T09:30:24Z

Maybe you could have a branch of talonfmt where you check in the compiled code so that it doesn't need to compile?

Was considering that, yes. The downside is that it's part of a different package, tree_sitter_talon. Guess I could build OS-specific versions of that package which include the compiles library? Anyone here know how to do that with setuptools?

pokey · 2022-08-03T10:53:14Z

Hmm. Looks like OS-specific native code builds for Python are still pretty painful today

Could maybe use wasm with wasmer?

wenkokke · 2022-08-03T16:49:55Z

Hmm. Looks like OS-specific native code builds for Python are still pretty painful today

Could maybe use wasm with wasmer?

Unfortunately, the tree_sitter package relies hard on the C bindings.

wenkokke · 2022-08-03T17:43:50Z

Set pre-commit to skip talonfmt on pre-commit.ci. Added pre-commit to GitHub Actions.

auscompgeek · 2022-08-04T08:16:57Z

It currently doesn't have C++ build tools, which breaks tree_sitter.

I took a look at a pre-commit.ci run, the build tools run fine, but the package tries to write to the installation directory at runtime, which is a big no-no since the virtual environment is immutable once installed.

If this will require a C++ compiler at runtime, this will be very annoying for people on Windows, as having Visual Studio installed is a lot more rare than having a C++ toolchain on UNIX systems.

apps/web/github.talon

misc/extensions.talon

wenkokke · 2022-08-17T18:09:13Z

Added blank lines after the match context, as requested.

rntz · 2022-08-18T17:10:53Z

I have no strong opinion on that case.

…

On Thu, 18 Aug 2022, 17:28 Wen Kokke, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In misc/extensions.talon <#947 (comment)>: > @@ -1 +1,2 @@ +- Do you all want to keep the - if it's already there, even if it's not needed? — Reply to this email directly, view it on GitHub <#947 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAD6DYVJFSTQ7TGJC3VRXTVZZQCHANCNFSM55IPZEBA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

wenkokke · 2022-08-23T22:06:50Z

Latest version:

supports key-binding declarations;
checks that formatting is idempotent and preserves the AST at runtime (the latter was suggested by @rntz);
resolves the ambiguity issues around match contexts in the parser, by using arbitrary lookahead in the lexer (suggested by @pokey and @rntz); and
simplifies the parsing of matches, no longer attempting to parse match contexts into logical formulas (suggested by @pokey).

I've also switched all the defaults over to the settings used here.

auscompgeek · 2022-09-03T14:17:29Z

.pre-commit-config.yaml

+      - id: remove-tabs
+        types: [file]
+        files: \.talon$
+        args: ["--whitespaces-count=4"]


Huh, it does seem like most of our .talon files use 4 space indents. We should fix the .editorconfig to do the same.

Opened #970 for this.

Would be cool if remove-tabs respected .editorconfig. I believe prettier does

wenkokke · 2022-09-04T14:25:29Z

Can we merge this? I don’t think there’s still any blocking issues.

rntz · 2022-09-04T16:51:58Z

We should probably decide at this point whether we're standardizing on 2 or 4 spaces for indentation in talon files. Per #970 we mostly use 4 spaces but the editorconfig file is set up to use 2 spaces. I prefer 2 spaces but I reckon I'm probably outvoted here :P. Still:

rntz · 2022-09-04T16:52:28Z

upvote for 2 spaces

rntz · 2022-09-04T16:52:34Z

upvote for 4 spaces

wenkokke · 2022-09-13T20:46:59Z

Okay, it's been open to a vote for 9 days. Can we merge this now?

phillco · 2022-09-13T21:45:27Z

I'd naturally wait for @rntz or @pokey, but this looks good to me. Might be worth to do in the grooming session this Saturday.

wenkokke · 2022-09-14T21:41:12Z

🎉🎉🎉

phillco · 2022-09-15T00:43:32Z

Awesome! Thanks a bunch for this change, Wen.

Now to merge up my fork :D

This omits the changes from #947 (446ec76) from blame for people who have the configuration set, so the reformatting changes don't affect blame.

This PR adds `talonfmt`, as well a tabs-to-spaces conversion, to pre-commit. The changes in the .talon should _absolutely_ be reviewed. Note that `talonfmt` supports alignment of context headers and one-line commands, but I've not enabled those options.

This omits the changes from talonhub#947 (446ec76) from blame for people who have the configuration set, so the reformatting changes don't affect blame.

This omits the changes from talonhub/community#947 (446ec764c9caa98973eacd7f792b6a087a1b635f) from blame for people who have the configuration set, so the reformatting changes don't affect blame.

Add remove-tabs for .talon files.

86002ac

rntz reviewed Aug 2, 2022

View reviewed changes

apps/i3wm/i3wm.talon Show resolved Hide resolved

apps/generic_debugger.talon Outdated Show resolved Hide resolved

wenkokke force-pushed the talonfmt branch from b7d92e8 to 4cf0b91 Compare August 2, 2022 22:51

wenkokke force-pushed the talonfmt branch from 4cf0b91 to b347eae Compare August 2, 2022 23:23

This was referenced Aug 2, 2022

Variant of "Add formatter for .talon files to pre-commit" with dynamic alignment #949

Closed

Variant of "Add formatter for .talon files to pre-commit" with fixed alignment #950

Closed

wenkokke force-pushed the talonfmt branch from e8c964e to 8bea3f9 Compare August 3, 2022 16:27

wenkokke force-pushed the talonfmt branch 3 times, most recently from 873e7d8 to 6a49b97 Compare August 3, 2022 17:41

pokey reviewed Aug 13, 2022

View reviewed changes

apps/web/github.talon Outdated Show resolved Hide resolved

pokey reviewed Aug 13, 2022

View reviewed changes

misc/extensions.talon Outdated Show resolved Hide resolved

wenkokke requested a review from pokey August 13, 2022 15:54

Add talonfmt 1.7.0 and format .talon files

c05257f

wenkokke force-pushed the talonfmt branch from efeb2e0 to c05257f Compare August 23, 2022 22:00

auscompgeek reviewed Sep 3, 2022

View reviewed changes

auscompgeek mentioned this pull request Sep 4, 2022

editorconfig: Default to 4 spaces #970

Merged

phillco approved these changes Sep 13, 2022

View reviewed changes

pokey merged commit 446ec76 into talonhub:main Sep 14, 2022

wenkokke deleted the talonfmt branch September 14, 2022 21:41

This was referenced Sep 15, 2022

all: reformat Talon using talonfmt phillco/talon-axkit#36

Merged

blame-ignore-revs: add talonfmt changes #975

Merged

rntz pushed a commit that referenced this pull request Sep 16, 2022

blame-ignore-revs: add talonfmt changes (#975)

c6d675e

This omits the changes from #947 (446ec76) from blame for people who have the configuration set, so the reformatting changes don't affect blame.

auscompgeek mentioned this pull request Sep 17, 2022

.talon file auto formatter #909

Closed

phillco mentioned this pull request Oct 2, 2022

talonfmt: raise maximum line length to 88 #990

Merged

pokey mentioned this pull request Oct 9, 2022

Workaround for number_small performance on old knausj version cursorless-dev/cursorless#1021

Closed

4 tasks

Add formatter for .talon files to pre-commit #947

Add formatter for .talon files to pre-commit #947

Conversation

wenkokke commented Aug 1, 2022 • edited

rntz commented Aug 1, 2022

wenkokke commented Aug 1, 2022 • edited

wenkokke commented Aug 1, 2022

pokey commented Aug 2, 2022

pokey commented Aug 2, 2022

rntz left a comment

Choose a reason for hiding this comment

pokey commented Aug 2, 2022

wenkokke commented Aug 2, 2022 • edited

wenkokke commented Aug 2, 2022 • edited

wenkokke commented Aug 2, 2022

wenkokke commented Aug 2, 2022 • edited

phillco commented Aug 2, 2022

wenkokke commented Aug 2, 2022

phillco commented Aug 2, 2022

wenkokke commented Aug 2, 2022

pokey commented Aug 3, 2022

wenkokke commented Aug 3, 2022 • edited

pokey commented Aug 3, 2022

wenkokke commented Aug 3, 2022 • edited

wenkokke commented Aug 3, 2022

auscompgeek commented Aug 4, 2022 • edited

wenkokke commented Aug 17, 2022

rntz commented Aug 18, 2022 via email

wenkokke commented Aug 23, 2022 • edited

auscompgeek Sep 3, 2022

Choose a reason for hiding this comment

auscompgeek Sep 4, 2022

Choose a reason for hiding this comment

pokey Sep 14, 2022

Choose a reason for hiding this comment

wenkokke commented Sep 4, 2022

rntz commented Sep 4, 2022

rntz commented Sep 4, 2022

rntz commented Sep 4, 2022

wenkokke commented Sep 13, 2022

phillco commented Sep 13, 2022 • edited

wenkokke commented Sep 14, 2022

phillco commented Sep 15, 2022 • edited

wenkokke commented Aug 1, 2022 •

edited

wenkokke commented Aug 1, 2022 •

edited

wenkokke commented Aug 2, 2022 •

edited

wenkokke commented Aug 2, 2022 •

edited

wenkokke commented Aug 2, 2022 •

edited

wenkokke commented Aug 3, 2022 •

edited

wenkokke commented Aug 3, 2022 •

edited

auscompgeek commented Aug 4, 2022 •

edited

wenkokke commented Aug 23, 2022 •

edited

phillco commented Sep 13, 2022 •

edited

phillco commented Sep 15, 2022 •

edited