Fix TypeScript error line/column

[Zarel]

Fixes #97

[Zarel]

I tested this on my own repository and it works!

Screenshot of it working:

[Zarel]

I'm surprised this hasn't gotten any attention after 17 days.

In the meantime, if you want this fixed in your local repository, it's a reasonably simple change:

[Zarel]

@ericsciple you self-assigned the bug this PR fixes; would you be interested in reviewing this?

[bryanmacfarlane]

Sorry. Been busy on some other changes. I'll have a look soon.

[chancancode]

I cannot human-parse this regex, but maybe this should just use the official $tsc pattern from vscode? Source: https://github.com/microsoft/vscode/blob/master/extensions/typescript-language-features/package.json#L974-L981 (it is not the same as this one)

[chancancode]

I have tested the official regex and can confirm that it works:

// commit this file to .github/tsc.json
{
  "problemMatcher": [
    {
      "owner": "tsc",
      "pattern": [
        {
          "name": "tsc",
          "regexp": "^([^\\s].*)[\\(:](\\d+)[,:](\\d+)(?:\\):\\s+|\\s+-\\s+)(error|warning|info)\\s+TS(\\d+)\\s*:\\s*(.*)$",
          "file": 1,
          "line": 2,
          "column": 3,
          "severity": 4,
          "code": 5,
          "message": 6
        }
      ]
    }
  ]
}

- name: Set up Node
  uses: actions/setup-node@v1
# Run this _after_ setup-node
- name: Set up tsc problem matcher
  run: echo "::add-matcher::${GITHUB_WORKSPACE}/.github/tsc.json"

[Zarel]

Here is my regex, free-spaced and syntax highlighted:

Here is the official pattern from TypeScript, free-spaced and syntax-highlighted:

A TypeScript error message looks like this:

lib/net.ts(48,26): error TS1005: '}' expected.

Both regexes support that pattern. Mine is modified from the old one in this repository, which supported a variable number of commas.

In "fancy" mode, a TypeScript error message looks like this:

lib/net.ts:48:26 - error TS1005: '}' expected.

48  			out += `${key}=${enc odeURIComponent('' + body[key])}`;
    			                     ~~~~~~~~~~~~~~~

The official regex also supports this format (although it should never be passed to GitHub Actions in this format, in practice).

[Zarel]

In conclusion, the official pattern is probably better.

Do you think the free-spaced annotated version should be put somewhere, for readability?

^
# filename
([^\s].*)
# line number in "(1,2)" or ":1:2 -" format
[\(:]
(\d+) [,:] (\d+)
(?:\):\s+|\s+-\s+)
# severity
(error|warning|info)
# error code
\s+ TS(\d+)
# ":"
\s* :
# error message
\s* (.*)
$

(It can be compiled into the version in the repository with https://zarel.github.io/regexfree/ )

[Zarel]

(One other difference is that the official pattern doesn't consider "TS" part of the error code. There's an argument that it should be kept, for googleability.)

[TimWolla]

This change works for me.

[TimWolla]

@bryanmacfarlane Is this one still on your radar?

[bryanmacfarlane]

@Zarel any interest in changing the PR to target main instead of master?

[Zarel]

@bryanmacfarlane Sure! I just did. Does this mean you're planning to merge this?

[bryanmacfarlane]

LGTM. @ericsciple can you have a quick look?

Also, side question - I think that regex was pulled (or created from) the one that VS Code uses in it's problem matchers. Anyone know what VS Code is currently using and how this PR compares to it?

[lumaxis]

@bryanmacfarlane As somebody mentioned in one of the comments above, this is the exact same regex that VS Code is currently using: https://github.com/microsoft/vscode/blob/8d9002679558432eca926b6bb4f767eaae6b1068/extensions/typescript-language-features/package.json#L1095-L1106

[ericsciple]

It might be good to add a unit test that loads the regexp and processes it against a line, asserts expected match groups

I'm worried if this change regresses something, at least we have a baseline unit test suite going forward.

Also i'm curious how this compares to vscode's built-in tsc problem matcher or their tsc-watch problem matcher. I briefly looked, but wan't able to find those built-in regexps in their repo.

[bryanmacfarlane]

@ericsciple looks like tests were added below at your request. have a quick look

[bryanmacfarlane]

@lumaxis are you ok to add unit test(s) per @ericsciple comment? I think we're good to merge after that.

[lumaxis]

@bryanmacfarlane Yea, that sounds like a great idea 👍🏼 Added tests in Zarel#1
Either @Zarel can merge my PR directly or @bryanmacfarlane can you merge/cherry-pick them into this branch with maintainer edit powers?

[Zarel]

@bryanmacfarlane Yea, that sounds like a great idea 👍🏼 Added tests in Zarel#1
Either @Zarel can merge my PR directly or @bryanmacfarlane can you merge/cherry-pick them into this branch with maintainer edit powers?

I merged your PR directly. I'm glad to see this is finally getting looked at! :)

[bryanmacfarlane]

LGTM. any objections @ericsciple ? (if not, I'll merge tomorrow)