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

Ensure contributed problem patterns are found correctly #12805

Merged
merged 2 commits into from
Aug 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/task/src/browser/task-configurations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import {
TaskDefinition,
TaskOutputPresentation,
TaskConfigurationScope,
TaskScope
TaskScope,
asVariableName
} from '../common';
import { TaskDefinitionRegistry } from './task-definition-registry';
import { ProvidedTaskConfigurations } from './provided-task-configurations';
Expand Down Expand Up @@ -349,7 +350,7 @@ export class TaskConfigurations implements Disposable {
} else if (task.problemMatcher) {
problemMatcher.push(task.problemMatcher.name!);
}
customization.problemMatcher = problemMatcher.map(name => name.startsWith('$') ? name : `$${name}`);
customization.problemMatcher = problemMatcher.map(asVariableName);
}
if (task.group) {
customization.group = task.group;
Expand Down
18 changes: 9 additions & 9 deletions packages/task/src/browser/task-problem-matcher-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,18 @@ import { Event, Emitter } from '@theia/core/lib/common';
import { Disposable, DisposableCollection } from '@theia/core/lib/common/disposable';
import {
ApplyToKind, FileLocationKind, NamedProblemMatcher,
ProblemPattern, ProblemMatcher, ProblemMatcherContribution, WatchingMatcher
ProblemPattern, ProblemMatcher, ProblemMatcherContribution, WatchingMatcher,
fromVariableName
} from '../common';
import { ProblemPatternRegistry } from './task-problem-pattern-registry';
import { Severity } from '@theia/core/lib/common/severity';
import { Deferred } from '@theia/core/lib/common/promise-util';

@injectable()
export class ProblemMatcherRegistry {

private readonly matchers = new Map<string, NamedProblemMatcher>();
private readyPromise: Promise<void>;
private readyPromise = new Deferred<void>();

@inject(ProblemPatternRegistry)
protected readonly problemPatternRegistry: ProblemPatternRegistry;
Expand All @@ -47,13 +49,13 @@ export class ProblemMatcherRegistry {
protected init(): void {
this.problemPatternRegistry.onReady().then(() => {
this.fillDefaults();
this.readyPromise = new Promise<void>((res, rej) => res(undefined));
this.readyPromise.resolve();
this.onDidChangeProblemMatcherEmitter.fire(undefined);
});
}

onReady(): Promise<void> {
return this.readyPromise;
return this.readyPromise.promise;
}

/**
Expand All @@ -73,6 +75,7 @@ export class ProblemMatcherRegistry {
this.doRegister(matcher, toDispose).then(() => this.onDidChangeProblemMatcherEmitter.fire(undefined));
return toDispose;
}

protected async doRegister(matcher: ProblemMatcherContribution, toDispose: DisposableCollection): Promise<void> {
const problemMatcher = await this.getProblemMatcherFromContribution(matcher);
if (toDispose.disposed) {
Expand All @@ -88,10 +91,7 @@ export class ProblemMatcherRegistry {
* @return the problem matcher. If the task definition is not found, `undefined` is returned.
*/
get(name: string): NamedProblemMatcher | undefined {
if (name.startsWith('$')) {
return this.matchers.get(name.slice(1));
}
return this.matchers.get(name);
return this.matchers.get(fromVariableName(name));
}

/**
Expand Down Expand Up @@ -132,7 +132,7 @@ export class ProblemMatcherRegistry {
if (matcher.pattern) {
if (typeof matcher.pattern === 'string') {
await this.problemPatternRegistry.onReady();
const registeredPattern = this.problemPatternRegistry.get(matcher.pattern);
const registeredPattern = this.problemPatternRegistry.get(fromVariableName(matcher.pattern));
if (Array.isArray(registeredPattern)) {
patterns.push(...registeredPattern);
} else if (!!registeredPattern) {
Expand Down
9 changes: 5 additions & 4 deletions packages/task/src/browser/task-problem-pattern-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,24 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { Disposable, DisposableCollection } from '@theia/core/lib/common/disposable';
import { Deferred } from '@theia/core/lib/common/promise-util';
import { injectable, postConstruct } from '@theia/core/shared/inversify';
import { NamedProblemPattern, ProblemLocationKind, ProblemPattern, ProblemPatternContribution } from '../common';
import { Disposable, DisposableCollection } from '@theia/core/lib/common/disposable';

@injectable()
export class ProblemPatternRegistry {
private readonly patterns = new Map<string, NamedProblemPattern | NamedProblemPattern[]>();
private readyPromise: Promise<void>;
private readyPromise = new Deferred<void>();

@postConstruct()
protected init(): void {
this.fillDefaults();
this.readyPromise = new Promise<void>((res, rej) => res(undefined));
this.readyPromise.resolve();
}

onReady(): Promise<void> {
return this.readyPromise;
return this.readyPromise.promise;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/task/src/browser/task-schema-updater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { inputsSchema } from '@theia/variable-resolver/lib/browser/variable-inpu
import URI from '@theia/core/lib/common/uri';
import { ProblemMatcherRegistry } from './task-problem-matcher-registry';
import { TaskDefinitionRegistry } from './task-definition-registry';
import { TaskServer } from '../common';
import { TaskServer, asVariableName } from '../common';
import { UserStorageUri } from '@theia/userstorage/lib/browser';
import { WorkspaceService } from '@theia/workspace/lib/browser';
import { JSONObject } from '@theia/core/shared/@phosphor/coreutils';
Expand Down Expand Up @@ -213,7 +213,7 @@ export class TaskSchemaUpdater implements JsonSchemaContribution {

/** Gets the most up-to-date names of problem matchers from the registry and update the task schema */
private updateProblemMatcherNames(): void {
const matcherNames = this.problemMatcherRegistry.getAll().map(m => m.name.startsWith('$') ? m.name : `$${m.name}`);
const matcherNames = this.problemMatcherRegistry.getAll().map(m => asVariableName(m.name));
problemMatcherNames.length = 0;
problemMatcherNames.push(...matcherNames);
this.update();
Expand Down
15 changes: 6 additions & 9 deletions packages/task/src/browser/task-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ import {
TaskInfo,
TaskOutputPresentation,
TaskOutputProcessedEvent,
TaskServer
TaskServer,
asVariableName
} from '../common';
import { TaskWatcher } from '../common/task-watcher';
import { ProvidedTaskConfigurations } from './provided-task-configurations';
Expand Down Expand Up @@ -908,13 +909,9 @@ export class TaskService implements TaskConfigurationClient {
async updateTaskConfiguration(token: number, task: TaskConfiguration, update: { [name: string]: any }): Promise<void> {
if (update.problemMatcher) {
if (Array.isArray(update.problemMatcher)) {
update.problemMatcher.forEach((name, index) => {
if (!name.startsWith('$')) {
update.problemMatcher[index] = `$${update.problemMatcher[index]}`;
}
});
} else if (!update.problemMatcher.startsWith('$')) {
update.problemMatcher = `$${update.problemMatcher}`;
update.problemMatcher.forEach((_name, index) => update.problemMatcher[index] = asVariableName(update.problemMatcher[index]));
} else {
update.problemMatcher = asVariableName(update.problemMatcher);
}
}
this.taskConfigurations.updateTaskConfig(token, task, update);
Expand Down Expand Up @@ -1041,7 +1038,7 @@ export class TaskService implements TaskConfigurationClient {
({
label: matcher.label,
value: { problemMatchers: [matcher] },
description: matcher.name.startsWith('$') ? matcher.name : `$${matcher.name}`
description: asVariableName(matcher.name)
})
));
return items;
Expand Down
1 change: 1 addition & 0 deletions packages/task/src/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@
export * from './task-protocol';
export * from './task-watcher';
export * from './problem-matcher-protocol';
export * from './task-util';
43 changes: 43 additions & 0 deletions packages/task/src/common/task-util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// *****************************************************************************
// Copyright (C) 2023 EclipseSource and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// http://www.eclipse.org/legal/epl-2.0.
//
// This Source Code may also be made available under the following Secondary
// Licenses when the conditions for such availability set forth in the Eclipse
// Public License v. 2.0 are satisfied: GNU General Public License, version 2
// with the GNU Classpath Exception which is available at
// https://www.gnu.org/software/classpath/license.html.
//
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
// *****************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add some tsdocs to these new util methods?

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 point! I pushed an update with some comments.


/**
* Converts the given standard name to a variable name starting with '$' if not already present.
*
* Variable names are used, for instance, to reference problem matchers, within task configurations.
*
* @param name standard name
* @returns variable name with leading '$' if not already present.
*
* @see {@link fromVariableName} for the reverse conversion.
*/
export function asVariableName(name: string): string {
return name.startsWith('$') ? name : `$${name}`;
}

/**
* Converts a given variable name to a standard name, effectively removing a leading '$' if present.
*
* Standard names are used, for instance, in registries to store variable objects
*
* @param name variable name
* @returns variable name without leading '$' if present.
*
* @see {@link asVariableName} for the reverse conversion.
*/
export function fromVariableName(name: string): string {
return name.startsWith('$') ? name.slice(1) : name;
}
Loading