Repository: rokucommunity/brighterscript
Pull request #565 (2022-04-08 18:20:55)

Fix manifest issues
Makes manifest parsing more accurate. Re-enable and fix manifest tests.


Patch:
diff --git a/src/Program.ts b/src/Program.ts
index 0aee9b44..9b691e2b 100644
--- a/src/Program.ts
+++ b/src/Program.ts
@@ -16,7 +16,6 @@ import { DependencyGraph } from './DependencyGraph';
 import { Logger, LogLevel } from './Logger';
 import chalk from 'chalk';
 import { globalFile } from './globalCallables';
-import type { ManifestValue } from './preprocessor/Manifest';
 import { parseManifest } from './preprocessor/Manifest';
 import { URI } from 'vscode-uri';
 import PluginInterface from './PluginInterface';
@@ -1434,7 +1433,7 @@ export class Program {
         }
         return this._manifest;
     }
-    private _manifest: Map<string, ManifestValue>;
+    private _manifest: Map<string, string>;
 
     public dispose() {
         for (let filePath in this.files) {
diff --git a/src/preprocessor/Manifest.spec.ts b/src/preprocessor/Manifest.spec.ts
index f127fb95..0d7253b9 100644
--- a/src/preprocessor/Manifest.spec.ts
+++ b/src/preprocessor/Manifest.spec.ts
@@ -1,125 +1,112 @@
-// import * as fsExtra from 'fs';
-// import { expect } from 'chai';
-// import { getManifest, getBsConst } from './Manifest';
-// import { createSandbox, SinonSandbox } from 'sinon';
-// let sinon: SinonSandbox;
-
-// describe('manifest support', () => {
-//     beforeEach(() => {
-//         sinon = createSandbox();
-//     });
-//     afterEach(() => {
-//         sinon.restore();
-//     });
-
-//     describe('manifest parser', () => {
-//         it('returns an empty map if manifest not found', async () => {
-//             // sinon.stub(fsExtra, 'readFile').returns(<any>
-//             //     Promise.reject(
-//             //         new Error('File not found')
-//             //     )
-//             // );
-//             sinon.stub(fsExtra, 'readFile').returns(<any>
-//                 Promise.reject(
-//                     new Error('File not found')
-//                 )
-//             );
-
-//             return expect(await getManifest('/no/manifest/here')).to.eql(new Map());
-//         });
-
-//         it('rejects key-value pairs with no \'=\'', () => {
-//             fs.readFile.mockImplementation((filename, encoding, cb) => cb(/* no error */ null, 'no_equal')
-//             );
-
-//             return expect(getManifest('/has/key/but/no/equal')).rejects.toThrowError(
-//                 'No \'=\' detected'
-//             );
-//         });
-
-//         it('ignores comments', () => {
-//             fs.readFile.mockImplementation((filename, encoding, cb) => cb(/* no error */ null, '# this line is ignored!')
-//             );
-
-//             return expect(getManifest('/has/a/manifest')).resolves.to.eql(new Map());
-//         });
-
-//         it('ignores empty keys and values', () => {
-//             fs.readFile.mockImplementation((filename, encoding, cb) => cb(/* no error */ null, ['  =lorem', 'ipsum=  '].join('\n'))
-//             );
-
-//             return expect(getManifest('/has/blank/keys/and/values')).resolves.to.eql(new Map());
-//         });
-
-//         it('trims whitespace from keys and values', () => {
-//             fs.readFile.mockImplementation((filename, encoding, cb) => cb(/* no error */ null, '    key = value    ')
-//             );
-
-//             return expect(getManifest('/has/extra/whitespace')).resolves.to.eql(
-//                 new Map([['key', 'value']])
-//             );
-//         });
-
-//         it('parses key-value pairs', () => {
-//             fs.readFile.mockImplementation((filename, encoding, cb) => cb(
-//                 /* no error */ null,
-//                 ['foo=bar=baz', 'lorem=true', 'five=5', 'six=6.000', 'version=1.2.3'].join('\n')
-//             )
-//             );
-
-//             return expect(getManifest('/has/a/manifest')).resolves.to.eql(
-//                 new Map([
-//                     ['foo', 'bar=baz'],
-//                     ['lorem', true],
-//                     ['five', 5],
-//                     ['six', 6],
-//                     ['version', '1.2.3']
-//                 ])
-//             );
-//         });
-//     });
-
-//     describe('bs_const parser', () => {
-//         it('returns an empty map if \'bs_const\' isn\'t found', () => {
-//             let manifest = new Map([['containsBsConst', false]]);
-//             expect(getBsConst(manifest)).to.eql(new Map());
-//         });
-
-//         it('requires a string value for \'bs_const\' attributes', () => {
-//             let manifest = new Map([['bs_const', 1.2345]]);
-//             expect(() => getBsConst(manifest)).toThrowError('Invalid bs_const right-hand side');
-//         });
-
-//         it('ignores empty key-value pairs', () => {
-//             let manifest = new Map([['bs_const', ';;;;']]);
-//             expect(getBsConst(manifest)).to.eql(new Map());
-//         });
-
-//         it('rejects key-value pairs with no \'=\'', () => {
-//             let manifest = new Map([['bs_const', 'i-have-no-equal']]);
-//             expect(() => getBsConst(manifest)).toThrowError('No \'=\' detected');
-//         });
-
-//         it('trims whitespace from keys and values', () => {
-//             let manifest = new Map([['bs_const', '   key   =  true  ']]);
-//             expect(getBsConst(manifest)).to.eql(new Map([['key', true]]));
-//         });
-
-//         it('rejects non-boolean values', () => {
-//             let manifest = new Map([['bs_const', 'string=word']]);
-
-//             expect(() => getBsConst(manifest)).to.throw;
-//         });
-
-//         it('allows case-insensitive booleans', () => {
-//             let manifest = new Map([['bs_const', 'foo=true;bar=FalSE']]);
-
-//             expect(getBsConst(manifest)).to.eql(
-//                 new Map([
-//                     ['foo', true],
-//                     ['bar', false]
-//                 ])
-//             );
-//         });
-//     });
-// });
+import * as fsExtra from 'fs';
+import { expect } from 'chai';
+import { getManifest, getBsConst, parseManifest } from './Manifest';
+import { createSandbox } from 'sinon';
+import { expectThrows, mapToObject, objectToMap, trim } from '../testHelpers.spec';
+const sinon = createSandbox();
+
+describe('manifest support', () => {
+    beforeEach(() => {
+        sinon.restore();
+    });
+    afterEach(() => {
+        sinon.restore();
+    });
+
+    describe('manifest parser', () => {
+        it('returns an empty map if manifest not found', async () => {
+            sinon.stub(fsExtra, 'readFile').returns(<any>
+                Promise.reject(
+                    new Error('File not found')
+                )
+            );
+
+            return expect(await getManifest('/no/manifest/here')).to.eql(new Map());
+        });
+
+        it('rejects key-value pairs with no \'=\'', () => {
+            expectThrows(() => {
+                parseManifest('key');
+            });
+        });
+
+        it('ignores comments', () => {
+            return expect(parseManifest('')).to.eql(new Map());
+        });
+
+        it('retains whitespace for keys and values', () => {
+            expect(
+                mapToObject(
+                    parseManifest(' leading interum and trailing key spaces = value ')
+                )
+            ).to.eql({
+                ' leading interum and trailing key spaces ': ' value '
+            });
+        });
+
+        it('does not convert values to primitives', () => {
+            expect(
+                mapToObject(
+                    parseManifest(trim`
+                        name=bob
+                        age=12
+                        enabled=true
+                        height=1.5
+                    `)
+                )
+            ).to.eql({
+                name: 'bob',
+                age: '12',
+                enabled: 'true',
+                height: '1.5'
+            });
+        });
+    });
+
+    describe('bs_const parser', () => {
+        function test(manifest: string, expected) {
+            expect(
+                getBsConst(
+                    parseManifest(manifest)
+                )
+            ).to.eql(
+                objectToMap(expected)
+            );
+        }
+
+        it('returns an empty map if \'bs_const\' isn\'t found', () => {
+            test('', new Map());
+        });
+
+        it('ignores empty key-value pairs', () => {
+            test('bs_const=;;;;', new Map());
+        });
+
+        it('rejects key-value pairs with no \'=\'', () => {
+            expectThrows(() => test(`bs_const=i-have-no-equal`, {}), `No '=' detected for key i-have-no-equal.  bs_const constants must be of the form 'key=value'.`);
+        });
+
+        it('trims whitespace from keys and values', () => {
+            let manifest = new Map([['bs_const', '   key   =  true  ']]);
+            expect(getBsConst(manifest)).to.eql(new Map([['key', true]]));
+        });
+
+        it('rejects non-boolean values', () => {
+            const manifest = new Map([['bs_const', 'string=word']]);
+            expectThrows(() => {
+                getBsConst(manifest);
+            });
+        });
+
+        it('allows case-insensitive booleans', () => {
+            let manifest = new Map([['bs_const', 'foo=true;bar=FalSE']]);
+
+            expect(getBsConst(manifest)).to.eql(
+                new Map([
+                    ['foo', true],
+                    ['bar', false]
+                ])
+            );
+        });
+    });
+});
diff --git a/src/preprocessor/Manifest.ts b/src/preprocessor/Manifest.ts
index 9bb0fb10..acad0663 100644
--- a/src/preprocessor/Manifest.ts
+++ b/src/preprocessor/Manifest.ts
@@ -1,11 +1,10 @@
 import * as fsExtra from 'fs-extra';
 import * as path from 'path';
 
-/** The set of possible value types in a `manifest` file's `key=value` pair. */
-export type ManifestValue = number | string | boolean;
-
-/** A map containing the data from a `manifest` file. */
-export type Manifest = Map<string, ManifestValue>;
+/**
+ * A map containing the data from a `manifest` file.
+ */
+export type Manifest = Map<string, string>;
 
 /**
  * Attempts to read a `manifest` file, parsing its contents into a map of string to JavaScript
@@ -34,49 +33,26 @@ export async function getManifest(rootDir: string): Promise<Manifest> {
  *          representing the manifest file's contents
  */
 export function parseManifest(contents: string) {
-    let keyValuePairs = contents
-        // for each line
-        .split('\n')
-        // remove leading/trailing whitespace
-        .map(line => line.trim())
-        // separate keys and values
-        .map((line, index) => {
-            // skip empty lines and comments
-            if (line === '' || line.startsWith('#')) {
-                return ['', ''];
-            }
+    const lines = contents.split(/\r?\n/g);
+    const result = new Map<string, string>();
+    for (let i = 0; i < lines.length; i++) {
+        const line = lines[i];
+        // skip empty lines and comments
+        if (line.trim() === '' || line.trim().startsWith('#')) {
+            continue;
+        }
 
-            let equals = line.indexOf('=');
-            if (equals === -1) {
-                throw new Error(
-                    `[manifest:${index +
-                    1}] No '=' detected.  Manifest attributes must be of the form 'key=value'.`
-                );
-            }
-            return [line.slice(0, equals), line.slice(equals + 1)];
-        })
-        // keep only non-empty keys and values
-        .filter(([key, value]) => key && value)
-        // remove leading/trailing whitespace from keys and values
-        .map(([key, value]) => [key.trim(), value.trim()])
-        // convert value to boolean, integer, or leave as string
-        .map(([key, value]): [string, ManifestValue] => {
-            if (value.toLowerCase() === 'true') {
-                return [key, true];
-            }
-            if (value.toLowerCase() === 'false') {
-                return [key, false];
-            }
-
-            let maybeNumber = Number.parseInt(value);
-            // if it's not a number, it's just a string
-            if (Number.isNaN(maybeNumber)) {
-                return [key, value];
-            }
-            return [key, maybeNumber];
-        });
-
-    return new Map<string, ManifestValue>(keyValuePairs);
+        let equalIndex = line.indexOf('=');
+        if (equalIndex === -1) {
+            throw new Error(
+                `[manifest:${i + 1}] No '=' detected.  Manifest attributes must be of the form 'key=value'.`
+            );
+        }
+        const key = line.slice(0, equalIndex);
+        const value = line.slice(equalIndex + 1);
+        result.set(key, value);
+    }
+    return result;
 }
 
 /**
diff --git a/src/types/InterfaceType.spec.ts b/src/types/InterfaceType.spec.ts
index 4185a098..9a174ae6 100644
--- a/src/types/InterfaceType.spec.ts
+++ b/src/types/InterfaceType.spec.ts
@@ -1,5 +1,6 @@
 import { expect } from 'chai';
 import { assert } from 'sinon';
+import { objectToMap } from '../testHelpers.spec';
 import type { BscType } from './BscType';
 import { DynamicType } from './DynamicType';
 import { IntegerType } from './IntegerType';
@@ -185,9 +186,7 @@ describe('InterfaceType', () => {
 
 function iface(members: Record<string, BscType>) {
     return new InterfaceType(
-        new Map(
-            Object.entries(members)
-        )
+        objectToMap(members)
     );
 }
 


Test patch:
diff --git a/src/testHelpers.spec.ts b/src/testHelpers.spec.ts
index f0b4b7cd..d5806931 100644
--- a/src/testHelpers.spec.ts
+++ b/src/testHelpers.spec.ts
@@ -309,3 +309,34 @@ export function expectCompletionsExcludes(completions: CompletionItem[], expecte
         }
     }
 }
+
+export function expectThrows(callback: () => any, expectedMessage = undefined, failedTestMessage = 'Expected to throw but did not') {
+    let wasExceptionThrown = false;
+    try {
+        callback();
+    } catch (e) {
+        wasExceptionThrown = true;
+        if (expectedMessage) {
+            expect((e as any).message).to.eql(expectedMessage);
+        }
+    }
+    if (wasExceptionThrown === false) {
+        throw new Error(failedTestMessage);
+    }
+}
+
+export function objectToMap<T>(obj: Record<string, T>) {
+    const result = new Map<string, T>();
+    for (let key in obj) {
+        result.set(key, obj[key]);
+    }
+    return result;
+}
+
+export function mapToObject<T>(map: Map<any, T>) {
+    const result = {} as Record<string, T>;
+    for (let [key, value] of map) {
+        result[key] = value;
+    }
+    return result;
+}