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

Fix TypeScript error line/column #125

Merged
merged 5 commits into from Feb 17, 2021
Merged

Fix TypeScript error line/column #125

merged 5 commits into from Feb 17, 2021

Conversation

@Zarel
Copy link
Contributor

@Zarel Zarel commented Mar 24, 2020

Fixes #97

@Zarel
Copy link
Contributor Author

@Zarel Zarel commented Mar 24, 2020

I tested this on my own repository and it works!

Screenshot of it working:

image

@Zarel
Copy link
Contributor Author

@Zarel Zarel commented Apr 9, 2020

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:

image

@Zarel
Copy link
Contributor Author

@Zarel Zarel commented Apr 9, 2020

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

@bryanmacfarlane bryanmacfarlane self-assigned this May 2, 2020
@bryanmacfarlane
Copy link
Member

@bryanmacfarlane bryanmacfarlane commented May 2, 2020

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

"regexp": "^(?:\\s+\\d+\\>)?([^\\s].*)\\((\\d+)(?:,(\\d+)(?:,\\d+(?:,\\d+)?)?)?\\)\\s*:\\s+(error|warning|info)\\s+(\\w{1,2}\\d+)\\s*:\\s*(.*)$",
"file": 1,
"location": 2,
"severity": 3,
"code": 4,
"message": 5
"line": 2,
"column": 3,
"severity": 4,
"code": 5,
"message": 6
Comment on lines 7 to 13

This comment has been minimized.

@chancancode

chancancode May 28, 2020

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)

This comment has been minimized.

@chancancode

chancancode May 28, 2020

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"

This comment has been minimized.

@Zarel

Zarel May 29, 2020
Author Contributor

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

image

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

image

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).

This comment has been minimized.

@Zarel

Zarel May 29, 2020
Author Contributor

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/ )

This comment has been minimized.

@Zarel

Zarel May 29, 2020
Author Contributor

(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.)

.github/tsc.json Outdated Show resolved Hide resolved
Copy link

@TimWolla TimWolla left a comment

This change works for me.

@TimWolla
Copy link

@TimWolla TimWolla commented Oct 27, 2020

@bryanmacfarlane Is this one still on your radar?

@Zarel Zarel force-pushed the Zarel:patch-1 branch from 8f76801 to c7026a4 Nov 16, 2020
@bryanmacfarlane
Copy link
Member

@bryanmacfarlane bryanmacfarlane commented Jan 20, 2021

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

@Zarel Zarel changed the base branch from master to main Jan 20, 2021
@Zarel
Copy link
Contributor Author

@Zarel Zarel commented Jan 20, 2021

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

@bryanmacfarlane bryanmacfarlane requested a review from ericsciple Feb 16, 2021
@bryanmacfarlane
Copy link
Member

@bryanmacfarlane bryanmacfarlane commented Feb 16, 2021

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
Copy link
Contributor

@lumaxis lumaxis commented Feb 16, 2021

@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

@@ -4,14 +4,15 @@
"owner": "tsc",
"pattern": [
{
"regexp": "^(?:\\s+\\d+\\>)?([^\\s].*)\\((\\d+|\\d+,\\d+|\\d+,\\d+,\\d+,\\d+)\\)\\s*:\\s+(error|warning|info)\\s+(\\w{1,2}\\d+)\\s*:\\s*(.*)$",
"regexp": "^([^\\s].*)[\\(:](\\d+)[,:](\\d+)(?:\\):\\s+|\\s+-\\s+)(error|warning|info)\\s+TS(\\d+)\\s*:\\s*(.*)$",

This comment has been minimized.

@ericsciple

ericsciple Feb 16, 2021
Collaborator

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.

This comment has been minimized.

@bryanmacfarlane

bryanmacfarlane Feb 17, 2021
Member

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

@bryanmacfarlane
Copy link
Member

@bryanmacfarlane bryanmacfarlane commented Feb 16, 2021

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

@lumaxis
Copy link
Contributor

@lumaxis lumaxis commented Feb 16, 2021

@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
Copy link
Contributor Author

@Zarel Zarel commented Feb 17, 2021

@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
Copy link
Member

@bryanmacfarlane bryanmacfarlane commented Feb 17, 2021

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

This was referenced Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants