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

stephen-carter-at-sf · 2024-12-05T20:27:22Z

No description provided.

stephen-carter-at-sf · 2024-12-05T20:34:30Z

src/commands/code-analyzer/config.ts


 	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) => {


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.

stephen-carter-at-sf · 2024-12-05T20:36:20Z

src/lib/messages.ts

 	ResultsWriter = 'results-writer',
 	RunAction = 'run-action',
 	RunCommand = 'run-command',
-	RunSummaryViewer = 'run-summary-viewer',


Cleaned this up. Was unused.

stephen-carter-at-sf · 2024-12-05T20:38:03Z

src/lib/models/ConfigModel.ts

+	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;


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.

stephen-carter-at-sf · 2024-12-05T20:39:33Z

src/lib/models/ConfigModel.ts

-			return this.toYamlUncheckedFieldWithInlineComment(fieldName, null, commentText);
-		} else if (JSON.stringify(userValue) === JSON.stringify(defaultValue)) {
-			return this.toYamlUncheckedField(fieldName, userValue);
+	private toYamlFieldUsingFieldDescription(fieldName: string, resolvedValue: unknown, fieldDescription: ConfigFieldDescription): string {


Using the name "resolvedValue" instead of "userValue" because it is post-resolution of the config... which mean it could be a calculated value like our python_command value for example.

stephen-carter-at-sf · 2024-12-05T20:40:48Z

src/lib/models/ConfigModel.ts

 	private getDefaultRuleFor(engineName: string, ruleName: string): Rule|null {
 		try {
-			return this.defaultContext.rules.getRule(engineName, ruleName);
+			return this.allDefaultRules.getRule(engineName, ruleName);


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.

stephen-carter-at-sf · 2024-12-05T20:41:40Z

src/lib/models/ConfigModel.ts


 		let yamlCode: string = '\n' +
-			this.toYamlSectionHeadingComment(engineConfigDescriptor.overview!) + '\n' +
+			this.toYamlSectionHeadingComment(engineConfigDescriptor.overview) + '\n' +


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).

stephen-carter-at-sf · 2024-12-05T20:43:10Z

src/lib/models/ConfigModel.ts

 	}
 }

 class PlainYamlFormatter extends YamlFormatter {


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)

stephen-carter-at-sf · 2024-12-05T20:43:39Z

...n.test.ts/override-configurations/StubEngine2_forConfigWithRelativePathScenario.yml.goldfile


    # Some description for top_field
-    top_field: # Modified from: null
+    top_field: # Modified from: {}


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

stephen-carter-at-sf · 2024-12-05T20:44:37Z

test/lib/actions/ConfigAction.test.ts

 				{prop: 'config_root'},
 				{prop: 'log_folder'}
-			])(`When derivable property $prop input is non-null and non-default, it is rendered as-is with no comment`, async ({prop}) => {
+			])(`When derivable property $prop input is non-null and non-default, it is rendered as-is`, async ({prop}) => {


This test description was incorrect. There is a comment - which is "#modified from: ". So fixed it.

stephen-carter-at-sf · 2024-12-05T20:45:46Z

test/lib/actions/ConfigAction.test.ts

 					.replace('__DUMMY_LOG_FOLDER__', parentOfCurrentDirectory)
-					.replace('__DUMMY_DEFAULT_CONFIG_ROOT__', JSON.stringify(defaultConfig.getConfigRoot()))
-					.replace('__DUMMY_DEFAULT_LOG_FOLDER__', JSON.stringify(defaultConfig.getLogFolder()))
+					.replace('__DUMMY_DEFAULT_CONFIG_ROOT__', JSON.stringify(null))


This is the only behavior change. If the user sets their own config_root or log_folder... there is no need to show the old calculated value... we just show "null" instead as I believe we should. For example, who cares if they see that by default we would have chosen a random temporary directory. So adding in the "Modified from: " here was better to just add in "null" as the value.

jfeingold35 · 2024-12-05T20:51:55Z

test/lib/actions/ConfigAction.test.ts

 					.replace('__DUMMY_LOG_FOLDER__', parentOfCurrentDirectory)
-					.replace('__DUMMY_DEFAULT_CONFIG_ROOT__', JSON.stringify(defaultConfig.getConfigRoot()))
-					.replace('__DUMMY_DEFAULT_LOG_FOLDER__', JSON.stringify(defaultConfig.getLogFolder()))
+					.replace('__DUMMY_DEFAULT_CONFIG_ROOT__', JSON.stringify(null))


JSON.stringify(null) feels weird to me.

Yeah... I was a little lazy here. I could have just put 'null'.

…at fixes python_command issue

stephen-carter-at-sf requested review from jag-j and jfeingold35 December 5, 2024 20:27

stephen-carter-at-sf commented Dec 5, 2024

View reviewed changes

stephen-carter-at-sf force-pushed the sc/W-17293079 branch from c08aa17 to f064bf7 Compare December 5, 2024 20:46

jfeingold35 approved these changes Dec 5, 2024

View reviewed changes

stephen-carter-at-sf added 5 commits December 5, 2024 16:06

CHANGE: @W-17293079@: Update dependencies, refactor config command th…

3ae4cd6

…at fixes python_command issue

Some refactoring

f3cb27f

Final polish

219e83a

Simplify comments for config based fields

a8be747

FIX

abbf926

stephen-carter-at-sf force-pushed the sc/W-17293079 branch from 730f86b to abbf926 Compare December 5, 2024 21:07

stephen-carter-at-sf merged commit f105bfe into dev-5 Dec 5, 2024
12 checks passed

stephen-carter-at-sf deleted the sc/W-17293079 branch December 5, 2024 21:08

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

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

Uh oh!

Conversation

stephen-carter-at-sf commented Dec 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephen-carter-at-sf Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stephen-carter-at-sf Dec 5, 2024 •

edited

Loading