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 #62531 add negative text filtering to Problems #115351

Merged
merged 5 commits into from
Mar 5, 2021

Conversation

gjsjohnmurray
Copy link
Contributor

This PR fixes #62531

If the first character of the filter field value is ! then only problems that don't match the rest of the value will display.

To test:

  1. Set up a situation where there are multiple problems listed and no filter is specified.
  2. Enter a text filter that removes some of the problems.
  3. Then prepend a ! to your filter and confirm you see only the problems that the previous step had omitted.

@gjsjohnmurray
Copy link
Contributor Author

@sandy081 please review this.

@sandy081 sandy081 self-requested a review January 29, 2021 10:43
@sandy081 sandy081 added this to the February 2021 milestone Jan 29, 2021
@gjsjohnmurray
Copy link
Contributor Author

@sandy081 I think it'd be good to get this merged early in the life of 1.54 so it can get some miles on the clock 🙏

@sandy081
Copy link
Member

sandy081 commented Feb 9, 2021

Sorry for the delay. I will do first thing reviewing this tomorrow.

if (this.options.textFilter && uriMatches) {
return { visibility: true, data: { type: FilterDataType.ResourceMarkers, uriMatches } };
const negate = this.options.textFilter.startsWith('!');
const textFilter = this.options.textFilter.substring(negate ? 1 : 0);
Copy link
Member

Choose a reason for hiding this comment

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

Seems like textFilter & negate options are being computed outside and everywhere. Instead I would suggest to move these semantics into FilterOptions and change textFilter type to {text: string, negate: boolean} | undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I have pushed the changes.

@sandy081 sandy081 added the error-list Problems view label Feb 10, 2021
if (anyMatched && negate) {
return false;
} else if (anyMatched || negate) {
return negate ? false : { visibility: true, data: { type: FilterDataType.RelatedInformation, uriMatches: uriMatches || [], messageMatches: messageMatches || [] } };
Copy link
Member

Choose a reason for hiding this comment

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

Is it an oversight here to return false when negate?

Copy link
Member

Choose a reason for hiding this comment

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

@gjsjohnmurray Not sure if you have seen this comment?

@joaomoreno joaomoreno changed the base branch from master to main February 15, 2021 08:51
@sandy081 sandy081 modified the milestones: February 2021, March 2021 Feb 25, 2021
@sandy081 sandy081 merged commit afae259 into microsoft:main Mar 5, 2021
@sandy081
Copy link
Member

sandy081 commented Mar 5, 2021

@gjsjohnmurray Thanks for the changes. Merging the PR.

@gjsjohnmurray gjsjohnmurray deleted the fix-62531 branch March 5, 2021 09:33
@GitMensch
Copy link
Contributor

That's one of the long awaited "minor issues" that comes up quite regularly. Thanks for having it done so far. Of course people will wonder about combined filters next "include/" !"inc1.ext" !"inc2.ext"(all problems in all includes but not inc1 and inc2)...
I suggest to either add/adjust that before the release, or have the documentation be explicit about that combinations are either "all applied" or "all suppressed"

@sandy081
Copy link
Member

sandy081 commented Mar 5, 2021

@GitMensch not sure what you mean by your filter "include/" !"inc1.ext" !"inc2.ext" - this negative filtering support is added only for text filters - for eg do not show problems with text folder - for resource we already support globs (inclusive and exclusive)

@GitMensch
Copy link
Contributor

Sounds good. That remark was about a combined filtering in general:

  • file: only in folders that end in include
  • file: not files named "inc1.ext" or "inc2.ext"
  • text "overflow"
  • negated filter "possible"

(that was a help request from a co-worker some weeks ago to show all overflow warnings but filter out the ones that explicit said something about "possible overflow" [without recompiling everything])

@GitMensch
Copy link
Contributor

When I see all problems in a huge workspace I sometimes conclude to not or only investigate two (or three or...) issues.

problem1 problem2 shows no problem, problem1, problem2, problem1 + problem2, problem1 || problem2 and "problem1" also show no issues, so the negation can't work either (as the common text filter it applies to the complete filter text).

Is "combined filtering" (which doesn't seems to be possible yet) in the pipeline? I did not found an issue for this with "filter pane" search.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-list Problems view
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support negative filters on text
3 participants