Use `yaml` package when selecting build configs #351

wwlorey · 2021-04-12T21:27:04Z

Yaml values without quotes are supported now as well:

Fixes #349

src/commands/openYAMLConfigFile.ts

neelip · 2021-04-16T20:46:26Z

src/commands/openYAMLConfigFile.ts

+                // The range returned from the `yaml` package doesn't include newlines
+                // So count newlines and include them in the range we return
+                for (const char of configDocumentText.slice(0, buildConfigOffset)) {
+                    newlines += char === '\n' ? 1 : 0;


There seems to be another way to do this without the for-loop.
Will, you might have already looked at this, but wondering if we can use this method instead?
eemeli/yaml#127 (comment)

Also, should we track this issue? In case there is an improvement from the yaml side.

Thanks for the suggestion - now this is using only the yaml package to get ranges. I don't think we need to track the issue though. From the looks of it they've already rejected the feature request to include newline counts in ranges

ejizba

I'm generally only okay with disabling lint on a per-line basis. In this case, we can just fix the lint problems instead of disabling them - see suggestions

ejizba · 2021-04-16T23:14:42Z

src/commands/openYAMLConfigFile.ts

+    type YamlNode = YAMLMap | YAMLSeq | Pair | undefined;
+    const yamlNodes: YamlNode[] = [];
+    let yamlNode: YamlNode = parsedYaml.get('jobs');


ejizba · 2021-04-16T23:17:20Z

src/commands/openYAMLConfigFile.ts

+        if (yamlNode.hasOwnProperty('key') && yamlNode['key'].value === buildConfigToSelect && yamlNode.hasOwnProperty('value')) {
+            const configValue = <Scalar>yamlNode['value'];
+            const range = <number[] | undefined>configValue.range;


Fixing lint errors

Suggested change

if (yamlNode.hasOwnProperty('key') && yamlNode['key'].value === buildConfigToSelect && yamlNode.hasOwnProperty('value')) {

const configValue = <Scalar>yamlNode['value'];

const range = <number[] | undefined>configValue.range;

if ('key' in yamlNode && (<Scalar>yamlNode.key).value === buildConfigToSelect && 'value' in yamlNode) {

const configValue = <Scalar>yamlNode.value;

const range = configValue.range;

ejizba · 2021-04-16T23:19:18Z

src/commands/openYAMLConfigFile.ts

+        } else if (yamlNode.hasOwnProperty('items')) {
+            yamlNodes.push(...yamlNode['items'])
+        } else if (yamlNode.hasOwnProperty('value') && yamlNode['value'].hasOwnProperty('items')) {
+            yamlNodes.push(...yamlNode['value']['items'])
+        }


Fixing lint errors and seems easier to pop yamlNode.value onto the queue rather than dig into items in multiple places

Suggested change

} else if (yamlNode.hasOwnProperty('items')) {

yamlNodes.push(...yamlNode['items'])

} else if (yamlNode.hasOwnProperty('value') && yamlNode['value'].hasOwnProperty('items')) {

yamlNodes.push(...yamlNode['value']['items'])

}

} else if ('items' in yamlNode) {

yamlNodes.push(...yamlNode.items);

} else if ('value' in yamlNode && typeof yamlNode.value === 'object') {

yamlNodes.push(yamlNode.value)

}

ejizba · 2021-04-16T23:28:57Z

src/commands/openYAMLConfigFile.ts

-async function getSelection(configDocument: TextDocument, buildConfigToSelect: BuildConfig): Promise<Range | undefined> {
-    const configRegex: RegExp = new RegExp(`${buildConfigToSelect}:`);
+/* eslint-disable @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access, no-prototype-builtins, @typescript-eslint/no-unsafe-assignment */
+export async function getSelection(configDocument: TextDocument, buildConfigToSelect: BuildConfig): Promise<Range | undefined> {


Since the code is a bit complex and has some sketchy "any" types, I'd probably rename this to tryGetSelection and add a big try/catch that just returns undefined if an error happens. This function already allows | undefined in the return type anyways

ejizba · 2021-04-16T23:35:19Z

test/selectBuildConfigs.test.ts

+        const workspacePath: string = getWorkspacePath('testWorkspace');
+
+        test(title, async () => {
+            const configDocument: TextDocument = await workspace.openTextDocument(join(workspacePath, 'testWorkflows', testCase.workflowFileName));


Rather than use separate yml files in the "testWorkflows" directory, I'd prefer you just put the test data in this file (like, as a const), for a few reasons:

Using the file system and/or VS Code APIs can slow tests down. It's worth it for end-to-end tests, but overkill for unit tests IMO

You can easily manipulate the test data with template literals and whatnot

Idk just seems easier to read when the test data is in the same file.

I feel like the only downside is this file can get pretty long, but it just doesn't bother me 🤷‍♂️. One way to help with that is to scope your test data down to smaller cases. For example, you might have one test running against an "actual" yml file seen in the wild, but the other tests are usually just testing specific edge cases without the full "yml" file necessary.

nturinski · 2021-04-22T20:57:45Z

test/selectBuildConfigs.test.ts

+interface ISelectBuildConfigTestCase {
+    workflowIndex: number;
+    buildConfig: BuildConfig;
+    expectedSelection: undefined | {


nit: I don't think I've ever seen anyone list undefined first in a | type haha

nturinski · 2021-04-22T21:12:37Z

test/selectBuildConfigs.test.ts

+                expectedSelection = new Range(expectedStart, expectedEnd);
+            }
+
+            assert.ok(expectedSelection && selection?.isEqual(expectedSelection) || selection === expectedSelection, 'Actual and expected selections do not match');


You should be able to use assert.strictEquals() here. Using assert.ok seems kind of roundabout here.

I went with assert.ok because comparing two ranges uses their isEqual() function. I updated the assert to hopefully make it look more purposeful

Ahh, okay, makse sense.

nturinski · 2021-04-22T21:13:40Z

test/selectBuildConfigs.test.ts

+          azure_static_web_apps_api_token: $\{{ secrets.AZURE_STATIC_WEB_APPS_API_TOKEN_AMBITIOUS_ROCK_0D992521E }}
+          action: "close"
+
+`];


Could we try location paths with special characters and maybe some foreign characters? (Chinese or Korean or something seems plausible)

Select entire line when clicking GitHub config

68dd520

wwlorey requested a review from a team as a code owner April 12, 2021 21:27

ejizba reviewed Apr 12, 2021

View reviewed changes

src/commands/openYAMLConfigFile.ts Show resolved Hide resolved

src/commands/openYAMLConfigFile.ts Outdated Show resolved Hide resolved

William Lorey added 4 commits April 15, 2021 17:28

Use yaml package to parse workflow file, add tests

4f3e967

Merge branch 'main' into wwl/range

6bd0aa6

Revert main.js

e9585f2

Don't use EOL

f59e34c

neelip reviewed Apr 16, 2021

View reviewed changes

ejizba reviewed Apr 16, 2021

View reviewed changes

William Lorey added 2 commits April 19, 2021 13:21

Use CSTNode range, remove lint exceptions, add try/catch

c7f88c4

Update tests

c665ddf

wwlorey changed the title ~~Select entire line when clicking GitHub config tree item~~ Use yaml package when selecting build configs Apr 21, 2021

nturinski reviewed Apr 22, 2021

View reviewed changes

Update type, assert, and add special chars test case

7bff217

nturinski approved these changes Apr 22, 2021

View reviewed changes

wwlorey merged commit 8843883 into main Apr 22, 2021

wwlorey deleted the wwl/range branch April 22, 2021 23:20

github-actions bot locked and limited conversation to collaborators Jun 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `yaml` package when selecting build configs #351

Use `yaml` package when selecting build configs #351

wwlorey commented Apr 12, 2021 •

edited

Loading

neelip Apr 16, 2021

wwlorey Apr 21, 2021

ejizba left a comment

ejizba Apr 16, 2021

ejizba Apr 16, 2021

ejizba Apr 16, 2021

ejizba Apr 16, 2021

ejizba Apr 16, 2021

nturinski Apr 22, 2021

nturinski Apr 22, 2021

wwlorey Apr 22, 2021

nturinski Apr 22, 2021

nturinski Apr 22, 2021

Use yaml package when selecting build configs #351

Use yaml package when selecting build configs #351

Conversation

wwlorey commented Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejizba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `yaml` package when selecting build configs #351

Use `yaml` package when selecting build configs #351

wwlorey commented Apr 12, 2021 •

edited

Loading