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

Contribute problem matcher #24114

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Conversation

anthonykim1
Copy link

@anthonykim1 anthonykim1 commented Sep 16, 2024

Resolves: #3828
Breaking #23953 down into two PR

  1. problem matcher --> make sure to cover case where there is invalid strings printed before the Error (e.g. NameError or ValueError)
  2. Whether we will replace 'Run In Terminal by contributing task with the problem matcher attached.

@anthonykim1 anthonykim1 added the feature-request Request for new features or functionality label Sep 16, 2024
@anthonykim1
Copy link
Author

anthonykim1 commented Sep 16, 2024

There seems to be some sort of caching problem happening. Very first time the task is ran with problem matcher, the appropriate error message are shown in problem tab, however if that same terminal window is reused for second round of task with the same problem matcher, it messes up (show the capturing of different string and not the appropriate error) in the problems tab.

Screen.Recording.2024-09-16.at.1.26.42.PM.mov

@anthonykim1
Copy link
Author

anthonykim1 commented Sep 16, 2024

With f2251b8 Problem matcher shows the correct problems in the problems tab, but ONLY for the very first task instance. After the first run of the task and if user decides to REUSE the task terminal, I think the problem matcher or task is getting confused and doesn't show any problems:

Screen.Recording.2024-09-16.at.1.40.07.PM.mov

This issue is filed here in vscode repo: microsoft/vscode#228883

package.json Outdated
"problemMatchers":
[
{
"name": "pythonCustomMatcher",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be something shorter. @cwebster-99 This name will be used in tasks.json for problem matching with python tasks. Can you suggest one for python?. I was thinking going with python.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python SGTM!

@anthonykim1 anthonykim1 marked this pull request as ready for review September 16, 2024 21:39
@vs-code-engineering vs-code-engineering bot added this to the September 2024 milestone Sep 16, 2024
rzhao271
rzhao271 previously approved these changes Sep 16, 2024
@anthonykim1 anthonykim1 added the skip package*.json package.json and package-lock.json don't both need updating label Sep 16, 2024
Copy link
Member

@karthiknadig karthiknadig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the severity and message group.

package.json Outdated
Comment on lines 95 to 96
"severity": 4,
"message": 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above regex seems to only have 2 groups. This will not extract message/severity as far as I understand.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid of message (this shouldnt have any effect because message defaults to 5), and got rid of severity entirely too and that doesnt seem to do anything too. 472103b

"message": 5
},
{
"regexp": "^\\s*(.*)\\s*$"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expression seems to capture all lines. What is the purpose of this?

Copy link
Author

@anthonykim1 anthonykim1 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem matcher will not pick up intended errors(NameError, ValueError, etc) if this is removed.
Generally it seems that array that is set for "pattern" is responsible for multi-line string of output that we(problem matcher) intends to parse.
For our case specifically, it will be responsible for capturing the output right above (error ---> the error is the one we intend to show in the problem panel); So this would allow us to correctly capture and show the error in the problems panel, and not the output above the error.
Screenshot 2024-09-16 at 9 39 21 PM

package.json Outdated Show resolved Hide resolved
@anthonykim1 anthonykim1 marked this pull request as draft September 17, 2024 00:05
Co-authored-by: Karthik Nadig <[email protected]>
@anthonykim1 anthonykim1 merged commit b59af57 into microsoft:main Sep 17, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-terminal feature-request Request for new features or functionality skip package*.json package.json and package-lock.json don't both need updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default Python Problem Matcher
4 participants