Establish a common format for all grammars for easier consumption

[mike-lischke]

May I suggest to establish a consistent formatting in the entire grammar repository? There's a wild mix of indentation (usually 3 and 4 spaces or tabs), alignments and so on.

I have written a formatter class for my ANTLR4 VS Code extension, which is highly configurable, so it's easy to adjust it to rules that we can agree upon here.

I can offer a reformat of the entire grammar repo and create a command line (Node.js) tool everyone can use to apply the format to a grammar file (for future pull requests).

Is there any interest for this?

[teverett]

There is a formatter already in place, here:

https://github.com/antlr/Antlr4Formatter

[mike-lischke]

It seems not to be used very often and I cannot find any formatting rules, but ok, never mind.

[teverett]

Well... hold on. You're right that it's not universally used. Additionally it's a Java command-line app which is frankly less convenient than VS Code. I know if there was a VS Code one, I would use it and I'm sure others would too.

So, perhaps we agree on a formatting standard, and deprecate the old Java one in favor of the more modern and more convenient one you've written?

[mike-lischke]

Sounds good. My ANTLR4 extension already does formatting on save, super easy. You can have formatting settings in your grammar too (I think that's the better way, instead of forcing people to change their VS Code settings when contributing a grammar change). A command line client is convenient if you want to reformat a larger number of grammars or want to automatically reformat in a CI pipeline etc.

[teverett]

You can use any of the grammars I wrote for examples of "standard" formatting, via Antlr4Formatter. Here's the 1st one in the tree, alphabetically

https://github.com/antlr/grammars-v4/blob/master/acme/acme.g4

[kaby76]

Yes, let's reformat the .g4's. Can the reformat tool be available as a command-line tool as well as through the VSCode IDE? A command-line tool can be used to batch reformat grammar files, and used to verify that the .g4 is formatted according to the standard.

There are other coding standards for the .g4's which should be considered (naming, use of string literals in parser rules, useless parentheses, unused symbols, undefined lexer symbols, ...).

[mike-lischke]

My plan was to create a node package, which can be installed and executed directly as an NPM script or globally in a terminal. Since the code of the formatter is TypeScript, it would always require Node.js for execution. Or someone ports that to Python (which would then require python to be installed...), but that's not my language of choice. A C++ version would also be possible, but the most simple approach is the TypeScript variant.

[kaby76]

This sounds great. Let us know how to call it. I can then fit it into the build.

[mike-lischke]

I created the formatter tool and tested it with the acme grammar from Tom. The PR is here. @teverett please review the diff and let me know if you want changes. I defined the formatting options in the grammar to stay as close as possible to your original grammar, with 2 notable differences:

• Indentation is now 4 spaces.
• The super long lines have been reformatted into much better readable blocks.

The repository for the tool is antlr-format. The tool works already when debugging from VS Code. I still have to finish the node package, but that shouldn't take long. If you guys want to play with it (maybe test different options) then clone the repo, open it in VS Code and select the "Debug main index file" launch config. The path to the grammar is hard coded in the config, so you have to change that for your own testing. Don't forget to npm i in the repo root before you start debugging.

[teverett]

@mike-lischke I prefer the formatting of your formatter. It genuinely increases readability. I'm honestly ambivalent about 3 spaces or 4, as long as its not tabs :) :)

[mike-lischke]

Well, the common agreement is to use an even number. Three spaces seems pretty odd. And yes, I also dislike tab chars for indentation in source files.

I'll prepare a new PR with all the 500+ grammars in this repo formatted like the acme.g4. The only change I plan yet is to make the max line length 150 instead of 250, as the latter is really too long.

[KvanTTT]

I suggest not changing formatting for all grammars since the resulting PR will be very big and hard for review. Also, a lot of changed lines spoil git blame.

[mike-lischke]

What do you suggest instead, Ivan? Doing it in tranches? Or not at all, just to keep git blame as it is? I guess the git history will keep enough info to tell who changed something before the reformat.

[KvanTTT]

Can be leave existing grammars as is but apply the new formatting rules for new grammars?

[kaby76]

When someone submits an initial revision of the grammar, who is going to validate that it follows the coding standard? Is it going to be tested via tool by the reviewer, or just eyeballed and declared "that looks just about right"?

For subsequent changes to a grammar, which coding standard is followed: the repo coding standard or the one set in the initial revision of the grammar by the author? How do we apply the coding standard: only to the change set, or to the whole grammar?

For some grammars, there is no consistent coding style. (plsql lexer single-line per rule; plsql lexer multi-line START_CMD or NOT_EQUAL_OP).

[KvanTTT]

Formatting style is defined individually for grammar, but we can publish some good style guidlines but they should not be mandatory.

[kaby76]

A review should be done via automated means. We need a thorough, precise, and efficient review. I would expect that the parse trees of the before and after grammar reformat to be identical (removing off-channel or skip). I don't know what the formatter does with comments and actions. I can't get it working (my system doesn't like the command for this step https://github.com/mike-lischke/antlr-format/blob/0dd5c6a2ebc86e882dfda5093dd4bca01ee09dcd/package.json#L22 and when tried at a bash shell fails to do a dynamic load of 'events' package).

[mike-lischke]

That's actually a great idea Ken. In fact this makes for a very nice unit test in the formatting tool. In general formatting is done on a token by token basis, which means only whitespace tokens (as defined in the ANTLRv4 grammar) are changed. So, from a functional standpoint, it shouldn't matter if the grammar is reformatted, not even for actions or comments. The formatter is able to reformat comments too, but this is often not working well (if it contains more than just text, like markup or code), so it's disabled by default in the antlr-format tool.

About running the tool: that format-test NPM script is just a placeholder. Instead try either test script which is also executed in Github or open the formatting.spec.ts file, select the Run current Jest test launch config and start debugging.

[mike-lischke]

FYI: I just released the first version of the antlr-format tool. The documentation contains details about its usage. If you have question about it start a discussion or open an issue in its repository. A PR for the reformatted grammars in this repo has been opened and we might have to discuss details further, like the fact that I have used slightly different rules for lexer grammars, because usually you want lexer rules to stay on a single line, while parser rules wrap over multiple lines.

The formatter can automatically add the used rules as comments to a grammar, so it's easy for me to re-run the formatting process (takes less than 2 seconds for all 516 grammars). Though it's a one time step. Once options are in a grammar, no new ones are added anymore.

@kaby76 For usage in a CI you can used the included terminal command (a binary Node.js command). I hope usage becomes clear when you read the documentation. If not, just ask.

About the parse tree check: I have not implemented/used that idea in the tool repo, because it would require to generate parsers for all grammars, one before and one after formatting. That would have gone way beyond what is acceptable for such a small tool. Once ANTLRng (the TS port) is available I can do all that in memory and may implement that additional check.

[mike-lischke]

I had to split the tool into two parts: the formatter library and a cli wrapper.

[KvanTTT]

Hi folks! Sorry, I'm against the idea of any automatic commits, particularly the commits that fix formatting. They spoil git history, git blame (it becomes hard to track original author of the certain line of code), and in fact they are out of control. We can check formatting during the reviewing process. Can be cancel this activity?

Also, actually I don't support the idea of the unified formatting for all grammars, it's not very significan. And it can be fixed manually or during reviewing process.

We have different authors and different licenses for grammars, I don't expect the same formatting for all grammars.

[KvanTTT]

Are you admins using tools to review the PR, or you just eyeballing it and saying "good enough"?

Currently it happens in that way. But maybe it's a time to improve the process. Actually, sometimes I don't like how fast MR is being merged, at least when I see obvious bugs. I appreciate user contributions, but I also like code quality, and I think several maintainer should accept changes but not one (probably I should discuss it with @teverett).

Or what about creating the correct formatting as part of the PR? Not an auto commit, but another patch in the PR.

It's a good idea. Currently we use squash merging strategy in ANTLR grammars repository. That's why it could be either manual or auto commit, it doesn't matter since they all will be squashed before the final merging.

[kaby76]

Right now, we have a new grammar PR. I ran the formatter, and it outputs this. diff.txt It does seem a bit to pass a diff or tell the guy to run the tool.

[mike-lischke]

Sorry, I can't parse your message @kaby76 :-). The diff looks good to me.

[kaby76]

Perhaps it's a trivial observation, but what I'm trying to say is that the grammars cannot be "fixed manually or during reviewing process", especially for brand new grammars. The diffs could be too large, or could contain unprintable characters. The author can't reliably type in by hand each and every character that needs to be changed in the .g4 file. The author is going to have to be: (1) handed the reformatted grammar; or (2) given diff output and told to use patch; or (3) be able to run antlr-format himself. There is no other way other than to have it automated because antlr-format adjusts the format precisely.

[mike-lischke]

Ah, ok, thanks. Yes, that's why I suggested to have the GH pipeline reformat the grammar and attach the generated diff (if there's any) to the PR, automatically. So it becomes part of the review (but is still automatically generated) and is pushed as part of that. Is that possible?