Skip to content

allow for problem patterns without a line#39936

Merged
dbaeumer merged 2 commits intomicrosoft:masterfrom
doudou:problem_patterns_without_a_line
Feb 9, 2018
Merged

allow for problem patterns without a line#39936
dbaeumer merged 2 commits intomicrosoft:masterfrom
doudou:problem_patterns_without_a_line

Conversation

@doudou
Copy link
Contributor

@doudou doudou commented Dec 8, 2017

Problem matchers currently must provide a line (and will be ignored if they don't). There's however sometimes problems that have no lines.

In these cases, using the matcher, assuming a line of 1, would be IMO a lot more useful than ignoring it.

Closes #39919

@doudou
Copy link
Contributor Author

doudou commented Dec 11, 2017

Will update to follow the discussion in #6538

@doudou
Copy link
Contributor Author

doudou commented Dec 11, 2017

First impressions/questions after trying to implement this (CC: @dbaeumer, since you're the one that proposed the 'kind' property in the first place)

  • single-line problem patterns seem to be a special case of multi-line patterns. Is that right, or am I missing something ? If it is the case, should I unify them (basically removing the SingleLine* stuff) ?
  • it seems to me that the 'kind' property would be better placed at the pattern level (rather than the matcher level). One weird-ish way to implement it would be to allow passing it only in the first pattern. A better option would be to change the internal pattern representation for a class that would hold a list of patterns and additional properties (in this case, the kind) property. However, ProblemPattern[] is used directly in exported interfaces. I don't know what's the policy there.

I would appreciate some guidance there ... Completely new to the codebase.

@doudou doudou force-pushed the problem_patterns_without_a_line branch from 67464af to 4f31fd8 Compare December 11, 2017 23:00
@msftclas
Copy link

msftclas commented Dec 11, 2017 •
edited
Loading

CLA assistant check
All CLA requirements met.

@doudou
Copy link
Contributor Author

doudou commented Dec 11, 2017

First take on it. Went the shortest route, that is requiring that 'kind' is within the first pattern in a multi-pattern. It's unit tested, but as of yet not tested "in daily use".

@dbaeumer dbaeumer assigned dbaeumer and unassigned sandy081 Dec 12, 2017
@dbaeumer
Copy link
Member

dbaeumer commented Dec 12, 2017

@doudou thanks a lot for looking into this!

Regarding your questions:

  • yes a SingleLineMatcher === MultiLineMatcher with patterns.length === 1. However I would not necessarily fold them since the common code is factored into the AbstractLineMatcher. And it made the SingleLineMatcher case easier to execute which is the 99% case of matchers. The file matcher case can IMO fully be handled in the AbstractLineMatcher.

  • Having the kind on the pattern makes sense. For multi line patterns we can spec that the kind property is only valid on the first entry as we spec that the loop property is only valid on the last entry.

Let me know what you think about it.

@doudou
Copy link
Contributor Author

doudou commented Dec 13, 2017

Let me know what you think about it.

Sounds reasonable, I'm going to stick to that.

The PR already implements it this way. Could you have a look ? I'm still testing, but I'm guessing that you might have some comments ;-)

dbaeumer
dbaeumer reviewed Dec 13, 2017
View reviewed changes
Copy link
Member

@dbaeumer dbaeumer left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the code and especially for the test! Highly appreciated.

I only have two minor comments

src/vs/platform/markers/common/problemMatcher.ts Outdated
Copy link
Member

@dbaeumer dbaeumer Dec 13, 2017

Choose a reason for hiding this comment

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

Could we fold this with the forEach from below. May be it would be easiest to turn the forEach into a for(pattern of values).

src/vs/platform/markers/common/problemMatcher.ts Outdated
Copy link
Member

@dbaeumer dbaeumer Dec 13, 2017

Choose a reason for hiding this comment

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

Can we add a comment to say that the value is only valid on the first entry. Like we do with loop.

@doudou doudou force-pushed the problem_patterns_without_a_line branch from 4f31fd8 to 8652645 Compare December 26, 2017 18:32
@doudou
Copy link
Contributor Author

doudou commented Dec 26, 2017

I only have two minor comments

Updated.

@doudou doudou mentioned this pull request Jan 3, 2018
Merged
2 tasks
@dbaeumer dbaeumer added this to the December 2017/January 2018 milestone Jan 8, 2018
@alexdima alexdima modified the milestones: January 2018, February 2018 Feb 2, 2018
@doudou
Copy link
Contributor Author

doudou commented Feb 8, 2018

Not in 1.20 :( Anything missing in there ?

@dbaeumer
Copy link
Member

dbaeumer commented Feb 9, 2018

No, nothing missing here. I was very busy with Language Pack support in 1.20 which required bigger changes in the loader and core code of VS Code. So I needed to shift this to February.

@dbaeumer dbaeumer merged commit 2434bf5 into microsoft:master Feb 9, 2018
@doudou doudou deleted the problem_patterns_without_a_line branch February 9, 2018 11:40
@dbaeumer
Copy link
Member

dbaeumer commented Feb 9, 2018

I merge the PR. Thanks for providing it.

@doudou
Copy link
Contributor Author

doudou commented Feb 9, 2018

Thanks ! And thanks for vscode ... the first IDE that made me leave vim in 10 years.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dbaeumer dbaeumer dbaeumer left review comments

Assignees

@dbaeumer dbaeumer

Labels

None yet

Projects

None yet

Milestone

February 2018

Development

Successfully merging this pull request may close these issues.

do not require a line in problem reports

5 participants

@doudou @msftclas @dbaeumer @alexdima @sandy081