Add GitHub annotations formatting option

[reiniscirpons]

This PR adds functionality requested in #63 . In particular it adds a --github-annotations flag, which, when toggled formats the output of gaplint to match the format expected by GitHub CI runners to display code annotations, see also https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#setting-an-error-message .

To do so, I refactored the existing diagnostic printing logic by writing a separate DiagnosticFormatter class, which allows us to easily create new formatters conforming to a custom format in a few lines of configuration code. For example, below is the default and github formatter specification:

_FORMATTERS = {
    "default": DiagnosticFormatter(
        has_column_format="{filename}:{line_range}:{column_range}: {message} [{code}/{name}]",
        no_column_format="{filename}:{line_range}: {message} [{code}/{name}]",
        line_range_format=("{}", "-", "{}"),
        column_range_format=("{}", "-", "{}"),
        line_offset=1,
        column_offset=1,
    ),
    "github": DiagnosticFormatter(
        has_column_format="::{diagnostic_type} file={filename},{line_range},{column_range},title=[{code}/{name}]::{message}",
        no_column_format="::{diagnostic_type} file={filename},{line_range},title=[{code}/{name}]::{message}",
        line_range_format=("line={}", ",", "endLine={}"),
        column_range_format=("col={}", ",", "endColumn={}"),
        line_offset=1,
        column_offset=1,
    ),
}

I also added an additional field to the Diagnostic class called diagnostic_type which stores information on whether the diagnostic is a warning or error (this is set by the calling _warn or _error functions respectively). This allows the github annotation to more accurately reflect the type of diagnostic encountered.

Finally, more tests were added to test the actual textual representation of diagnostics (this was previously not done) and the expected output pickling script was updated to work with the changes introduced in #59 . Expected output was repickled so that diagnostics would also store the newly added diagnostic_type field.

[reiniscirpons]

@fingolfin could you check if this does what you want and displays the annotations correctly in the github CI?

Running it with gaplint --github-annotations should provide basic annotations in the github format on a line-by-line basis. You can get more granularity with line and column range level annotations by running it as gaplint --github-annotations --ranges. Also, the way I implemented it the regular gaplint diagnostic messages are no longer output at all, and only the github annotations are produced.

Sample output of the first few warnings when running gaplint --github-annotations ./tests/input/test1.g:

::warning file=./tests/input/test1.g,line=18,title=[W046/unused-func-args]::Unused function arguments: arg
::warning file=./tests/input/test1.g,line=44,title=[W000/analyse-lvars]::Unused local variables: t
::warning file=./tests/input/test1.g,line=45,title=[W046/unused-func-args]::Unused function arguments: 1a1a, 1b, localt, x, y
::warning file=./tests/input/test1.g,line=48,title=[W000/analyse-lvars]::Unused local variables: t
::warning file=./tests/input/test1.g,line=60,title=[W046/unused-func-args]::Unused function arguments: z
::warning file=./tests/input/test1.g,line=63,title=[W000/analyse-lvars]::Variables assigned but never used: test
::warning file=./tests/input/test1.g,line=27,title=[W001/consecutive-empty-lines]::Consecutive empty lines
::warning file=./tests/input/test1.g,line=60,title=[W034/1-line-function]::One line function could be a lambda
::warning file=./tests/input/test1.g,line=3,title=[W009/not-enough-space-before-comment]::At least 2 spaces before comment
::warning file=./tests/input/test1.g,line=3,title=[W020/whitespace-op-plus]::Wrong whitespace around operator +
::warning file=./tests/input/test1.g,line=5,title=[W005/align-trailing-comments]::Unaligned comments in consecutive lines
::warning file=./tests/input/test1.g,line=5,title=[W009/not-enough-space-before-comment]::At least 2 spaces before comment

Sample output of the first few warnings when running gaplint --github-annotations --ranges ./tests/input/test1.g:

::warning file=./tests/input/test1.g,line=18,title=[W046/unused-func-args]::Unused function arguments: arg
::warning file=./tests/input/test1.g,line=44,title=[W000/analyse-lvars]::Unused local variables: t
::warning file=./tests/input/test1.g,line=45,title=[W046/unused-func-args]::Unused function arguments: 1a1a, 1b, localt, x, y
::warning file=./tests/input/test1.g,line=48,title=[W000/analyse-lvars]::Unused local variables: t
::warning file=./tests/input/test1.g,line=60,title=[W046/unused-func-args]::Unused function arguments: z
::warning file=./tests/input/test1.g,line=63,title=[W000/analyse-lvars]::Variables assigned but never used: test
::warning file=./tests/input/test1.g,line=27,endLine=30,col=10,endColumn=1,title=[W001/consecutive-empty-lines]::Consecutive empty lines
::warning file=./tests/input/test1.g,line=60,endLine=62,col=8,endColumn=4,title=[W034/1-line-function]::One line function could be a lambda
::warning file=./tests/input/test1.g,line=3,col=9,endColumn=12,title=[W009/not-enough-space-before-comment]::At least 2 spaces before comment
::warning file=./tests/input/test1.g,line=3,col=5,endColumn=7,title=[W020/whitespace-op-plus]::Wrong whitespace around operator +
::warning file=./tests/input/test1.g,line=5,col=26,endColumn=27,title=[W005/align-trailing-comments]::Unaligned comments in consecutive lines
::warning file=./tests/input/test1.g,line=5,col=24,endColumn=27,title=[W009/not-enough-space-before-comment]::At least 2 spaces before comment

[fingolfin]

This looks good, thank you!

But ultimately it should be tested under "real" conditions: I.e. set up a git repository with a GH workflow which invokes gaplint --github-annotations on pull requests, and then make a PR which introduces "bad" GAP code.

(And then perhaps someone can turn this into a prepackeged action, say gap-actions/gaplint or so -- @Joseph-Edwards already has some experience with how that works and perhaps could help with this additional request ;-) )

[reiniscirpons]

But ultimately it should be tested under "real" conditions: I.e. set up a git repository with a GH workflow which invokes gaplint --github-annotations on pull requests, and then make a PR which introduces "bad" GAP code.

Started doing so in a fork of the example gap package project, see PR here reiniscirpons/gap-gaplint-github-annotation-test#1 . It seems that no annotations are being generated for the newly added file, but curiously it does produce some (but not all) annotations for the existing PackageInfo.g file and no others. There may be some hidden assumptions about how the ranges etc are displayed. Or some specific setup is necessary to correctly find the filename?

It seems https://github.com/actions/runner/blob/main/src/Runner.Worker/ActionCommandManager.cs#L608 is the code in the GH actions runner that is responsible for creating the annotations.

[reiniscirpons]

Update: after trawling the web it seems that there is a limit of 10 annotations that get generated (see https://github.com/actions/toolkit/blob/main/docs/problem-matchers.md#limitations), this is not 10 annotation per file, its 10 annotations total. There is also no indication that there are more annotations that were not displayed. So it seems like, if this were to be used in CI workflows, its crucial to a) only run gaplint on modified or newly added files and not already existing files (as these eat up the limit too) and b) communicate that more warnings could be present to the end user to prevent frustrating whack-a-mole situations where developers end up fixing 10 lints at a time only to see more crop up every time they push.

[fingolfin]

Yeah it would important to either limit the annotations to ranges of code that were actually touched by the current commit; or to advise people to only use the action after they formatted their entire code base (but of course that's a somewhat limiting restriction, some projects may wish to migrate gradually shrug)

As to the limit of 10 annotations, I wasn't aware but that explains some bad experience I had in the past (where I indeed thought I'd fixed all and then I got more...)

But I'd try to address this as follows:

• the README of gaplint resp. gap-actions/gaplint (should someone create such an action) should explain this limitation explicitly
• perhaps one can hack things such that output of the warnings is delayed; if at the end we know there are more than 10 warnings, one could adjust the warnings to include a message of the form "warning 7 of 123" or "(only the first 10 warnings are shown, the other 113 are not)" or so
• Codecov, in addition to warnings, also posts a comment on the PRs (and update that comment over time) which can include things like information about the total number of warnings, etc. -- but I am don't whether (and if so, how) such a thing can be done with a plain GAP action (for the PackageDistro, we use https://github.com/peter-evans/create-or-update-comment for such things, but that's in a workflow, not inside of an action)