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.

Sign up for GitHub

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

CHANGE: @W-17293079@: Update dependencies, refactor config command that fixes python_command issue #1691

Merged
merged 5 commits into from
Dec 5, 2024
Merged

CHANGE: @W-17293079@: Update dependencies, refactor config command that fixes python_command issue #1691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ae4cd6
CHANGE: @W-17293079@: Update dependencies, refactor config command th…
stephen-carter-at-sf Dec 5, 2024
f3cb27f
Some refactoring
stephen-carter-at-sf Dec 5, 2024
219e83a
Final polish
stephen-carter-at-sf Dec 5, 2024
a8be747
Simplify comments for config based fields
stephen-carter-at-sf Dec 5, 2024
abbf926
FIX
stephen-carter-at-sf Dec 5, 2024
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
14 changes: 7 additions & 7 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
"bugs": "https://github.com/forcedotcom/sfdx-scanner/issues",
"dependencies": {
"@oclif/core": "^3.3.2",
"@salesforce/code-analyzer-core": "0.17.0",
"@salesforce/code-analyzer-engine-api": "0.14.0",
"@salesforce/code-analyzer-eslint-engine": "0.14.0",
"@salesforce/code-analyzer-flowtest-engine": "0.14.1",
"@salesforce/code-analyzer-pmd-engine": "0.14.0",
"@salesforce/code-analyzer-regex-engine": "0.14.0",
"@salesforce/code-analyzer-retirejs-engine": "0.14.0",
"@salesforce/code-analyzer-core": "0.18.0",
"@salesforce/code-analyzer-engine-api": "0.15.0",
"@salesforce/code-analyzer-eslint-engine": "0.15.0",
"@salesforce/code-analyzer-flowtest-engine": "0.15.0",
"@salesforce/code-analyzer-pmd-engine": "0.15.0",
"@salesforce/code-analyzer-regex-engine": "0.15.0",
"@salesforce/code-analyzer-retirejs-engine": "0.15.0",
"@salesforce/core": "^5",
"@salesforce/sf-plugins-core": "^5.0.4",
"@salesforce/ts-types": "^2.0.9",
Expand Down
5 changes: 0 additions & 5 deletions src/commands/code-analyzer/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {EnginePluginsFactoryImpl} from '../../lib/factories/EnginePluginsFactory
import {BundleName, getMessage, getMessages} from '../../lib/messages';
import {LogEventDisplayer} from '../../lib/listeners/LogEventListener';
import {RuleSelectionProgressSpinner} from '../../lib/listeners/ProgressEventListener';
import {AnnotatedConfigModel, ConfigContext} from '../../lib/models/ConfigModel';
import {Displayable, UxDisplay} from '../../lib/Display';

export default class ConfigCommand extends SfCommand<void> implements Displayable {
Expand Down Expand Up @@ -62,15 +61,11 @@ export default class ConfigCommand extends SfCommand<void> implements Displayabl

protected createDependencies(outputFile?: string): ConfigDependencies {
const uxDisplay: UxDisplay = new UxDisplay(this, this.spinner);
const modelGeneratorFunction = /* istanbul ignore next - Model tested separately */ (relevantEngines: Set<string>, userContext: ConfigContext, defaultContext: ConfigContext) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was an unnecessary dependency. No need to inject this dependency unless we have a reason to do so with regards to testing/etc... which we don't.

return AnnotatedConfigModel.fromSelection(relevantEngines, userContext, defaultContext);
};
const dependencies: ConfigDependencies = {
configFactory: new CodeAnalyzerConfigFactoryImpl(),
pluginsFactory: new EnginePluginsFactoryImpl(),
logEventListeners: [new LogEventDisplayer(uxDisplay)],
progressEventListeners: [new RuleSelectionProgressSpinner(uxDisplay)],
modelGenerator: modelGeneratorFunction,
actionSummaryViewer: new ConfigActionSummaryViewer(uxDisplay),
viewer: new ConfigStyledYamlViewer(uxDisplay)
};
Expand Down
15 changes: 2 additions & 13 deletions src/lib/actions/ConfigAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@ import {createWorkspace} from '../utils/WorkspaceUtil';
import {LogEventListener, LogEventLogger} from '../listeners/LogEventListener';
import {ProgressEventListener} from '../listeners/ProgressEventListener';
import {ConfigActionSummaryViewer} from '../viewers/ActionSummaryViewer';
import {ConfigModel, ConfigModelGeneratorFunction, ConfigContext} from '../models/ConfigModel';
import {AnnotatedConfigModel, ConfigModel} from '../models/ConfigModel';

export type ConfigDependencies = {
configFactory: CodeAnalyzerConfigFactory;
pluginsFactory: EnginePluginsFactory;
modelGenerator: ConfigModelGeneratorFunction;
logEventListeners: LogEventListener[];
progressEventListeners: ProgressEventListener[];
writer?: ConfigWriter;
Expand Down Expand Up @@ -109,17 +108,7 @@ export class ConfigAction {
// We need the Set of all Engines that returned rules for the user's selection on both the Default and User Cores.
const relevantEngines: Set<string> = new Set([...userRules.getEngineNames(), ...selectedDefaultRules.getEngineNames()]);

const userConfigContext: ConfigContext = {
config: userConfig,
core: userCore,
rules: userRules
};
const defaultConfigContext: ConfigContext = {
config: defaultConfig,
core: defaultCoreForAllRules,
rules: allDefaultRules
};
const configModel: ConfigModel = this.dependencies.modelGenerator(relevantEngines, userConfigContext, defaultConfigContext);
const configModel: ConfigModel = new AnnotatedConfigModel(userConfig, userCore, userRules, allDefaultRules, relevantEngines);

const fileWritten: boolean = this.dependencies.writer
? await this.dependencies.writer.write(configModel)
Expand Down
1 change: 0 additions & 1 deletion src/lib/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ export enum BundleName {
ResultsWriter = 'results-writer',
RunAction = 'run-action',
RunCommand = 'run-command',
RunSummaryViewer = 'run-summary-viewer',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaned this up. Was unused.

Shared = 'shared',
WorkspaceUtil = 'workspace-util'
}
Expand Down
147 changes: 79 additions & 68 deletions src/lib/models/ConfigModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import {
CodeAnalyzer,
CodeAnalyzerConfig,
ConfigDescription,
ConfigFieldDescription,
EngineConfig,
Rule,
RuleSelection,
SeverityLevel
Expand All @@ -20,50 +22,51 @@ export interface ConfigModel {
toFormattedOutput(format: OutputFormat): string;
}

export type ConfigContext = {
config: CodeAnalyzerConfig;
core: CodeAnalyzer;
rules: RuleSelection;
}

export type ConfigModelGeneratorFunction = (relevantEngines: Set<string>, userContext: ConfigContext, defaultContext: ConfigContext) => ConfigModel;

export class AnnotatedConfigModel implements ConfigModel {
private readonly relevantEngines: Set<string>;
private readonly userContext: ConfigContext;
private readonly defaultContext: ConfigContext;

private constructor(relevantEngines: Set<string>, userContext: ConfigContext, defaultContext: ConfigContext) {
private readonly config: CodeAnalyzerConfig; // TODO: It would be nice if we updated the CodeAnalyzer (in our core module) to just return its CodeAnalyzerConfig with a getter so we didn't need to pass it around
private readonly codeAnalyzer: CodeAnalyzer;
private readonly userRules: RuleSelection;
private readonly allDefaultRules: RuleSelection;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically I could have passed in a function that just gives me back the default rule since that's how we are using this. But it was easy enough to just pass this in.

But now we don't need to pass in all the values and config. So no more need for the ConfigContext stuff.


// Note that it is important that we calculate the relevant engines list based on (the user rule selection with no
// config) plus (user rule selection with user config) since we still want to show the "disable_engine" config value
// in the output if a user even if selects an engine that is currently disabled. But we don't want to the engine
// configs not associated with the user's rule selection, thus we can't use the engines from allDefaultRules.
private readonly relevantEngines: Set<string>;

constructor(config: CodeAnalyzerConfig, codeAnalyzer: CodeAnalyzer, userRules: RuleSelection, allDefaultRules: RuleSelection, relevantEngines: Set<string>) {
this.config = config;
this.codeAnalyzer = codeAnalyzer;
this.userRules = userRules;
this.allDefaultRules = allDefaultRules;
this.relevantEngines = relevantEngines;
this.userContext = userContext;
this.defaultContext = defaultContext;
}

toFormattedOutput(format: OutputFormat): string {
// istanbul ignore else: Should be impossible
if (format === OutputFormat.STYLED_YAML) {
return new StyledYamlFormatter(this.relevantEngines, this.userContext, this.defaultContext).toYaml();
return new StyledYamlFormatter(this.config, this.codeAnalyzer, this.userRules, this.allDefaultRules, this.relevantEngines).toYaml();
} else if (format === OutputFormat.RAW_YAML) {
return new PlainYamlFormatter(this.relevantEngines, this.userContext, this.defaultContext).toYaml();
return new PlainYamlFormatter(this.config, this.codeAnalyzer, this.userRules, this.allDefaultRules, this.relevantEngines).toYaml();
} else {
throw new Error(`Unsupported`)
}
}

public static fromSelection(relevantEngines: Set<string>, userContext: ConfigContext, defaultContext: ConfigContext): AnnotatedConfigModel {
return new AnnotatedConfigModel(relevantEngines, userContext, defaultContext);
}
}

abstract class YamlFormatter {
private readonly config: CodeAnalyzerConfig;
private readonly codeAnalyzer: CodeAnalyzer;
private readonly userRules: RuleSelection;
private readonly allDefaultRules: RuleSelection;
private readonly relevantEngines: Set<string>;
private readonly userContext: ConfigContext;
private readonly defaultContext: ConfigContext;

protected constructor(relevantEngines: Set<string>, userContext: ConfigContext, defaultContext: ConfigContext) {
protected constructor(config: CodeAnalyzerConfig, codeAnalyzer: CodeAnalyzer, userRules: RuleSelection, allDefaultRules: RuleSelection, relevantEngines: Set<string>) {
this.config = config;
this.codeAnalyzer = codeAnalyzer;
this.userRules = userRules;
this.allDefaultRules = allDefaultRules;
this.relevantEngines = relevantEngines;
this.userContext = userContext;
this.defaultContext = defaultContext;
}

protected abstract toYamlComment(commentText: string): string
Expand All @@ -83,50 +86,65 @@ abstract class YamlFormatter {
return yamlCode.replace(/(\r?\n|$)/, ` ${comment}$1`);
}

private toYamlField(fieldName: string, userValue: unknown, defaultValue: unknown): string {
if (looksLikeAPathValue(userValue) && userValue === defaultValue) {
// We special handle a path value when it is equal to the default value, making it equal null because
// chances are it is a derived file or folder value based on the specific environment that we do not want to
// actually want to hard code since checking in the config to CI/CD system may create a different value
const commentText: string = getMessage(BundleName.ConfigModel, 'template.last-calculated-as', [JSON.stringify(userValue)]);
return this.toYamlUncheckedFieldWithInlineComment(fieldName, null, commentText);
} else if (JSON.stringify(userValue) === JSON.stringify(defaultValue)) {
return this.toYamlUncheckedField(fieldName, userValue);
private toYamlFieldWithFieldDescription(fieldName: string, resolvedValue: unknown, fieldDescription: ConfigFieldDescription): string {
const resolvedValueJson: string = JSON.stringify(resolvedValue);
const defaultValueJson: string = JSON.stringify(fieldDescription.defaultValue);

let yamlField: string;
if (!fieldDescription.wasSuppliedByUser && resolvedValueJson !== defaultValueJson) {
// Whenever the user did not supply the value themselves but the resolved value is different from the
// default value, this means the value was not a "fixed" value but a value "calculated" at runtime.
// Since "calculated" values often depend on the specific environment, we do not want to actually hard code
// this value into the config since checking in the config to CI/CD system may create a different value.
const commentText: string = getMessage(BundleName.ConfigModel, 'template.last-calculated-as', [resolvedValueJson]);
yamlField = this.toYamlUncheckedFieldWithInlineComment(fieldName, fieldDescription.defaultValue, commentText);
} else {
yamlField = this.toYamlField(fieldName, resolvedValue, fieldDescription.defaultValue);
}

userValue = replaceAbsolutePathsWithRelativePathsWherePossible(userValue, this.userContext.config.getConfigRoot() + path.sep);
return this.toYamlComment(fieldDescription.descriptionText) + "\n" + yamlField
}

private toYamlField(fieldName: string, resolvedValue: unknown, defaultValue: unknown): string {
const resolvedValueJson: string = JSON.stringify(resolvedValue);
const defaultValueJson: string = JSON.stringify(defaultValue);

const commentText: string = getMessage(BundleName.ConfigModel, 'template.modified-from', [JSON.stringify(defaultValue)]);
return this.toYamlUncheckedFieldWithInlineComment(fieldName, userValue, commentText);
if (resolvedValueJson === defaultValueJson) {
return this.toYamlUncheckedField(fieldName, resolvedValue);
}

const commentText: string = getMessage(BundleName.ConfigModel, 'template.modified-from', [defaultValueJson]);
resolvedValue = replaceAbsolutePathsWithRelativePathsWherePossible(resolvedValue, this.config.getConfigRoot() + path.sep);
return this.toYamlUncheckedFieldWithInlineComment(fieldName, resolvedValue, commentText);
}

toYaml(): string {
const topLevelDescription: ConfigDescription = CodeAnalyzerConfig.getConfigDescription();
return this.toYamlSectionHeadingComment(topLevelDescription.overview!) + '\n' +
const topLevelDescription: ConfigDescription = this.config.getConfigDescription();
return this.toYamlSectionHeadingComment(topLevelDescription.overview) + '\n' +
'\n' +
this.toYamlComment(topLevelDescription.fieldDescriptions!.config_root) + '\n' +
this.toYamlField('config_root', this.userContext.config.getConfigRoot(), this.defaultContext.config.getConfigRoot()) + '\n' +
this.toYamlFieldWithFieldDescription('config_root', this.config.getConfigRoot(),
topLevelDescription.fieldDescriptions.config_root) + '\n' +
'\n' +
this.toYamlComment(topLevelDescription.fieldDescriptions!.log_folder) + '\n' +
this.toYamlField('log_folder', this.userContext.config.getLogFolder(), this.defaultContext.config.getLogFolder()) + '\n' +
this.toYamlFieldWithFieldDescription('log_folder', this.config.getLogFolder(),
topLevelDescription.fieldDescriptions.log_folder) + '\n' +
'\n' +
this.toYamlComment(topLevelDescription.fieldDescriptions!.rules) + '\n' +
this.toYamlComment(topLevelDescription.fieldDescriptions.rules.descriptionText) + '\n' +
this.toYamlRuleOverrides() + '\n' +
'\n' +
this.toYamlComment(topLevelDescription.fieldDescriptions!.engines) + '\n' +
this.toYamlComment(topLevelDescription.fieldDescriptions.engines.descriptionText) + '\n' +
this.toYamlEngineOverrides() + '\n' +
'\n' +
this.toYamlSectionHeadingComment(getMessage(BundleName.ConfigModel, 'template.common.end-of-config')) + '\n';
}

private toYamlRuleOverrides(): string {
if (this.userContext.rules.getCount() === 0) {
if (this.userRules.getCount() === 0) {
const commentText: string = getMessage(BundleName.ConfigModel, 'template.yaml.no-rules-selected');
return `rules: {} ${this.toYamlComment(commentText)}`;
}

let yamlCode: string = 'rules:\n';
for (const engineName of this.userContext.rules.getEngineNames()) {
for (const engineName of this.userRules.getEngineNames()) {
yamlCode += '\n';
yamlCode += indent(this.toYamlRuleOverridesForEngine(engineName), 2) + '\n';
}
Expand All @@ -138,7 +156,7 @@ abstract class YamlFormatter {
[engineName.toUpperCase()]);
let yamlCode: string = this.toYamlSectionHeadingComment(engineConfigHeader) + '\n';
yamlCode += `${engineName}:\n`;
for (const userRule of this.userContext.rules.getRulesFor(engineName)) {
for (const userRule of this.userRules.getRulesFor(engineName)) {
const defaultRule: Rule|null = this.getDefaultRuleFor(engineName, userRule.getName());
yamlCode += indent(this.toYamlRuleOverridesForRule(userRule, defaultRule), 2) + '\n';
}
Expand All @@ -147,7 +165,7 @@ abstract class YamlFormatter {

private getDefaultRuleFor(engineName: string, ruleName: string): Rule|null {
try {
return this.defaultContext.rules.getRule(engineName, ruleName);
return this.allDefaultRules.getRule(engineName, ruleName);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only place allDefaultRules is used. So like I said, I could have just passed in the getDefaultRuleFor method instead... but this was already here - so just keeping things as is.

} catch (e) {
// istanbul ignore next
return null;
Expand Down Expand Up @@ -176,31 +194,28 @@ abstract class YamlFormatter {
}

private toYamlEngineOverridesForEngine(engineName: string): string {
const engineConfigDescriptor = this.userContext.core.getEngineConfigDescription(engineName);
const userEngineConfig = this.userContext.core.getEngineConfig(engineName);
const defaultEngineConfig = this.defaultContext.core.getEngineConfig(engineName);
const engineConfigDescriptor: ConfigDescription = this.codeAnalyzer.getEngineConfigDescription(engineName);
const userEngineConfig: EngineConfig = this.codeAnalyzer.getEngineConfig(engineName);

let yamlCode: string = '\n' +
this.toYamlSectionHeadingComment(engineConfigDescriptor.overview!) + '\n' +
this.toYamlSectionHeadingComment(engineConfigDescriptor.overview) + '\n' +
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's nice to be able to remove all these ! now that the core side has its own ConfigDescription which makes these values required (because it always fills it in).

`${engineName}:\n`;
// By fiat, the field description will always include, at minimum, an entry for "disable_engine", so we can
// assume that the object is not undefined.
for (const configField of Object.keys(engineConfigDescriptor.fieldDescriptions!)) {
const fieldDescription = engineConfigDescriptor.fieldDescriptions![configField];
const defaultValue = defaultEngineConfig[configField] ?? null;
const userValue = userEngineConfig[configField] ?? defaultValue;
for (const configField of Object.keys(engineConfigDescriptor.fieldDescriptions)) {
const fieldDescription: ConfigFieldDescription = engineConfigDescriptor.fieldDescriptions[configField];
const resolvedValue = userEngineConfig[configField] ?? fieldDescription.defaultValue;
// Add a leading newline to visually break up the property from the previous one.
yamlCode += '\n' +
indent(this.toYamlComment(fieldDescription), 2) + '\n' +
indent(this.toYamlField(configField, userValue, defaultValue), 2) + '\n';
indent(this.toYamlFieldWithFieldDescription(configField, resolvedValue, fieldDescription), 2) + '\n';
}
return yamlCode.trimEnd();
}
}

class PlainYamlFormatter extends YamlFormatter {
Copy link
Collaborator Author

@stephen-carter-at-sf stephen-carter-at-sf Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There still is a bit of a code smell having to pass all these input arguments all around. Part of me thinks that we could maybe just give the Formatter the Model instead of passing the values through. But i'm on the fence because doing so means adding a bunch of getters to the model... so just leaving things as is.

(This is analogous to how our results object has a format method and the formatters inside of it then just receive the results object)

constructor(relevantEngines: Set<string>, userContext: ConfigContext, defaultContext: ConfigContext) {
super(relevantEngines, userContext, defaultContext);
constructor(config: CodeAnalyzerConfig, codeAnalyzer: CodeAnalyzer, userRules: RuleSelection, allDefaultRules: RuleSelection, relevantEngines: Set<string>) {
super(config, codeAnalyzer, userRules, allDefaultRules, relevantEngines);
}

protected toYamlComment(commentText: string): string {
Expand All @@ -209,19 +224,15 @@ class PlainYamlFormatter extends YamlFormatter {
}

class StyledYamlFormatter extends YamlFormatter {
constructor(relevantEngines: Set<string>, userContext: ConfigContext, defaultContext: ConfigContext) {
super(relevantEngines, userContext, defaultContext);
constructor(config: CodeAnalyzerConfig, codeAnalyzer: CodeAnalyzer, userRules: RuleSelection, allDefaultRules: RuleSelection, relevantEngines: Set<string>) {
super(config, codeAnalyzer, userRules, allDefaultRules, relevantEngines);
}

protected toYamlComment(commentText: string): string {
return commentText.replace(/^.*/gm, s => makeGrey(`# ${s}`));
}
}

function looksLikeAPathValue(value: unknown) {
return typeof(value) === 'string' && !value.includes('\n') && value.includes(path.sep);
}

function replaceAbsolutePathsWithRelativePathsWherePossible(value: unknown, parentFolder: string): unknown {
if (typeof value === 'string') {
// Check if the string starts with the parent folder
Expand Down
2 changes: 1 addition & 1 deletion ...est.ts/override-configurations/StubEngine2_forConfigWithRelativePathScenario.yml.goldfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
disable_engine: false

# Some description for top_field
top_field: # Modified from: null
top_field: # Modified from: {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in a more appropriate default for this field - which is why the goldfile changed.

sub_field:
- optional-input-config.yml
Loading
Loading