feat: basic implementation of command line config option #1080

saintsebastian · 2017-09-16T14:32:27Z

Fixes #775

Basic version. Some question:

would it be a good idea to use ./web-ext-config.js as default value for --config option since it needs one?
is program.js a good place to parse other config files from expected places like ~/web-ext-config.jsand ./web-ext-config.js?

coveralls · 2017-09-18T11:30:06Z

Coverage decreased (-0.3%) to 99.675% when pulling 5552269 on saintsebastian:config-option into 79ba4e6 on mozilla:master.

rpl · 2017-09-18T12:57:18Z

I took a look at what is the behavior currently implemented by the src/config.js module landed by #744
and here is some initial feedback related to the question:

if I remember correctly, there was the idea of loading the config values from "./.web-ext-config.js" and "~/.web-ext-config.js" if one of these files actually exist, but (at least in my opinion) the "config loading behavior" should be sightly different in the two scenarios:
- when the config file option has been explicitly passed over the command line, we should fail if the file doesn't exist
- but if we are loading some config file implicitly (e.g. from the current directory or from the home directory), then we should not fail if the file doesn't exist (nevertheless, we should emit a warning or a debug message)
yes, I think that program.js is where the config loading behaviors are going to be implemented (and based on the description in Configure the Program class to use config parsing helpers #775 I think that it is where @kumar303 is suggesting to implement it)

Another detail to take into account if we are going to load a config file implicitly is related to the default values displayed in the "web-ext --help" output, which can be different from the actual default values set once we load the config file implicitly, and it is likely to be confusing for the user
(and so we should at least make sure that we print the new loaded defaults and/or update the default values in the "web-ext --help" output).

@kumar303 Let me know how that sounds to you, e.g. if I'm not missing anything related to the plans for the "config file loading" behaviors.

rpl · 2017-09-18T11:59:22Z

src/program.js

+          options: this.options,
+          configFileName: argv.config,
+        });
+      } catch (error) {


By looking at the implementation and unit tests related to applyConfigToArgv, I think that we should not try...catch the exception that it throws, they seem to be meant to be raised to the user (e.g. UsageError for errors that can/should be fixed by the user and WebExtError for the unexpected errors).

@rpl tbh I wrapped it this way because instead of throwing i would get UnhandledPromiseRejectionWarning despite applyConfigToArgv being a sync function.

@saintsebastian that's because the execute method is an async function, it should be enough to just move the code that uses loadJSConfigFile and applyConfigToArgv inside the existent try...catch at line 150 of this patch (it was at line 135 before the changes applied by this patch), because it already implements the behavior that we want.

@rpl ah! now i see it. thanks! I will add tests and that should probably be it for this patch

kumar303 · 2017-09-18T18:30:31Z

@kumar303 Let me know how that sounds to you

Sounds good!

I would suggesting keeping the features in this patch small. For example, a great start would be to simply allow an option like --config that takes a path to a file, that's it. There are a lot of things to consider with auto-detecting config files -- we should look in the home directory, in the current directory, and maybe elsewhere (and each of these files will be merged together differently). The auto-detection can come later.

rpl · 2017-09-18T19:02:09Z

Sounds good!

Yeah, I agree completely, it is better to keep this PR simpler and defer that behaviors as follow up issues.

@saintsebastian do you mind to file this part (the "implicit config file loading") as a separate issue? (and mention #775 and this PR into its description so that they will be linked together by github).

saintsebastian · 2017-09-19T14:12:51Z

@rpl Hi! I assumed this issue 730 covers the file loading

rpl · 2017-09-19T14:22:11Z

@saintsebastian yeah, it is definitely the issue that I wanted, it was linked to a pull request that has been closed in favor of the one that has been landed, I've also updated its description to mention this PR.

coveralls · 2017-09-19T15:20:59Z

Coverage decreased (-0.1%) to 99.892% when pulling 295be34 on saintsebastian:config-option into 79ba4e6 on mozilla:master.

coveralls · 2017-09-19T15:27:53Z

Coverage decreased (-0.1%) to 99.892% when pulling 1c4ffd8 on saintsebastian:config-option into 79ba4e6 on mozilla:master.

coveralls · 2017-09-20T18:52:57Z

Coverage remained the same at 100.0% when pulling 03c9a6c on saintsebastian:config-option into 79ba4e6 on mozilla:master.

coveralls · 2017-09-20T19:12:21Z

Coverage remained the same at 100.0% when pulling 8c363e1 on saintsebastian:config-option into 436e26d on mozilla:master.

rpl

Thanks @saintsebastian!

This looks pretty near to its final goal, follows some additional tweaks (mostly related to the flow types and the test cases).

rpl · 2017-09-21T00:20:28Z

src/program.js

@@ -27,6 +31,8 @@ type ExecuteOptions = {
  systemProcess?: typeof process,
  logStream?: typeof defaultLogStream,
  getVersion?: Function,
+  applyConfigToArgv?: Function,


we can use the more specific type: typeof defaultLoadJSConfigFile
(Function would allow flow to consider as valid any function, besides its flow type signature).

same for loadJSConfigFile.

rpl · 2017-09-21T00:22:14Z

src/program.js

@@ -145,6 +153,17 @@ export class Program {
        });
      }

+      if (argv.config) {
+        const configLocation = path.resolve(argv.config);


Nit: if we call this configfileName, then we can use the shorter syntax at line 163.

rpl · 2017-09-21T00:23:17Z

src/program.js

+          argv,
+          configObject,
+          options: this.options,
+          configFileName: configLocation,


Let's move this line at line 161 to keep the keys in alphabetical order.

rpl · 2017-09-21T00:29:08Z

tests/unit/test.program.js

@@ -26,7 +26,7 @@ describe('program.Program', () => {
    const absolutePackageDir = path.join(__dirname, '..', '..');
    return program.execute(
      absolutePackageDir, {
-        getVersion: () => spy(),
+        getVersion: () => 'not-a-real-version',


argh... This is why it is better to use a more specific type signature instead of Function as a flow type signature :-)

Do you mind to file a follow up issue related to a more specific type signature for getVersion in "src/program.js" (or we can check how many flow errors it produces, if they are not too much we can fix it in this patch, on the contrary we can move it into a separate issue and pull request)

A reasonable flow type could be just () => string (a function which takes no parameters and returns a string).

rpl · 2017-09-21T12:53:53Z

tests/unit/test.program.js

+      .then(() => {
+        const options = fakeCommands.lint.firstCall.args[0];
+        assert.strictEqual(options.config, fakePath);
+        assert.ok(fakeLoadJSConfigFile.called);


We should turn most of these assertion into sinon assertion (http://sinonjs.org/releases/v2.3.5/assertions/)
and matchers (http://sinonjs.org/releases/v2.3.5/matchers/).

e.g. assert.ok(fakeLoadJSConfigFile.called) should be sinon.assert.calledOnce(fakeLoadJSConfigFile);

we should probably also add some additional test cases to cover the following scenarios:

the mocked loadJSConfigFile raises a UsageError or an unexpected exception

the mocked applyConfigToArgv raises a UsageError or a WebExtError or an unexpected exception

And check that the behavior is the one that we expect (e.g. we can assert that these errors should make the call to reject with the expected error, and we could also check that they have been printed on the stderr, e.g. by injecting a logStream defined as const logStream = fake(new ConsoleStream()); into execProgram)

saintsebastian · 2017-09-22T13:20:37Z

@rpl this is now ready for a review. I think new tests are very repetitive, so maybe you have some ideas how to make them better. Also there is Travis failure on this and my other pr, it seems to be npm related.

rpl

Hi @saintsebastian
follows some additional feedback related to the last round of changes.

I agree with you that there is a considerable amount of boilerplate code that is repeated on every one of the last group of tests, to avoid this, you can easily extract a new "test helper function" with the shared pieces and turn the parts that change into parameters of the "test helper function" (we use this approach in other test files too for the same reason).

Don't worry about the travis failure for now, it is related to the "nsp check" (which checks the npm package dependencies for known security vulnerabilities) and travis execute them before running the test suite.

rpl · 2017-09-28T14:46:55Z

src/program.js

@@ -26,7 +31,10 @@ type ExecuteOptions = {
  checkForUpdates?: Function,
  systemProcess?: typeof process,
  logStream?: typeof defaultLogStream,
+  logger?: Logger,


@saintsebastian I think that we should be able to make the test assertions related to the logged messages by turning on logStream.isCapturing and inspecting the logStream.capturedMessages array without injecting a fake logger instance:

web-ext/src/util/logger.js

Lines 44 to 45 in 177709b

isCapturing: boolean;

capturedMessages: Array<string>;

rpl · 2017-09-28T14:55:24Z

tests/unit/test.program.js

@@ -430,6 +433,203 @@ describe('program.main', () => {
      assert.strictEqual(options.shouldExitProgram, false);
    });
  });
+
+  it('applies config from config file when specified', () => {


how about "applies options from the specified config file"?

rpl · 2017-09-28T14:56:46Z

tests/unit/test.program.js

+      .then(() => {
+        const options = fakeCommands.lint.firstCall.args[0];
+        assert.strictEqual(options.config, fakePath);
+        sinon.assert.calledOnce(fakeLoadJSConfigFile);


I would prefer to see the sinon.assert.calledOnce and sinon.assert.calledWith related to the same spy grouped together.

rpl · 2017-09-28T14:57:41Z

tests/unit/test.program.js

+        sinon.assert.calledOnce(fakeLoadJSConfigFile);
+        sinon.assert.calledOnce(fakeApplyConfigToArgv);
+        sinon.assert.calledWith(fakeLoadJSConfigFile,
+          sinon.match(resolvedFakePath));


this line should be indented so that sinon.match... is aligned to the fakeLoadJSConfigFile from the previous line.

rpl · 2017-09-28T15:02:07Z

tests/unit/test.program.js

+      });
+  });
+
+  it('logs and throws when config file was not loaded', () => {


how about changing the description into something like 'throws an UsageError when the config file can't be loaded'?

rpl · 2017-09-28T15:05:10Z

tests/unit/test.program.js

+      }));
+  });
+
+  it('throws when config file loading fails', () => {


to make it a bit more clear which is the difference between this test and the previous one,
we could rephrase this description to "throws when config file loading raises an unexpected error"
(given that we only expect it to raise a UsageError in the known error cases)

rpl · 2017-09-28T15:06:13Z

tests/unit/test.program.js

+      });
+  });
+
+  it('logs and throws when fakeApplyConfigToArgv throws UsageError', () => {


how about "throws a UsageError when the loaded config can't be applied"?

rpl · 2017-09-28T15:11:42Z

tests/unit/test.program.js

+      }));
+  });
+
+  it('logs and throws when fakeApplyConfigToArgv throws WebExtError', () => {


@saintsebastian I'm wondering if we are really supposed to throw a WebExtError when a config file option is missing the type:

web-ext/src/config.js

Lines 52 to 55 in 177709b

if (options[decamelizedOptName].type === undefined) {

throw new WebExtError(

`Option: ${option} was defined without a type.`);

}

while we are raising a UsageError when the specified type is incorrect:

web-ext/src/config.js

Lines 61 to 65 in 177709b

if (optionType !== expectedType) {

throw new UsageError(`The config file at ${configFileName} specified ` +

`the type of "${option}" incorrectly as "${optionType}"` +

` (expected type: "${expectedType}")`);

}

it seems to me that we could/should throw a UsageError for both, @kumar303 what do you think about it?
(e.g. I'm wondering if this has been already evaluated when the config helper module has been created)

rpl · 2017-09-28T15:16:45Z

tests/unit/test.program.js

+      }));
+  });
+
+  it('logs and throws when fakeApplyConfigToArgv throws WebExtError', () => {


This seems to be testing mostly the same scenario of the test above it, but it should probably be the test case which explicitly test what is going to happen if "fakeApplyConfigToArgv throws an unexpected error", am I right?

coveralls · 2017-10-21T12:40:05Z

Coverage remained the same at 100.0% when pulling 67ea96f on saintsebastian:config-option into 436e26d on mozilla:master.

coveralls · 2017-10-21T13:26:27Z

Coverage remained the same at 100.0% when pulling afe0454 on saintsebastian:config-option into 436e26d on mozilla:master.

… them

saintsebastian · 2017-10-21T13:33:40Z

Hey @rpl sorry it took me so long to get to this PR

I tried using consoleStream.startCapturing(), this however still only works with injected logger dependency.
about your concern on use of WebExtError I added the last commit with the changes you suggested, which we can always throw away in case @kumar303 thinks there is a good reason to use this type of error.

Thanks for cleaning up my wording and my tests!

rpl · 2017-10-30T14:30:45Z

tests/unit/test.program.js

-import {ConsoleStream} from '../../src/util/logger';
-
+import {
+  consoleStream,


@saintsebastian nit, an inline comment here could be useful here (just to confirm to the next developer that reads this test file that we are importing both consoleStream and ConsoleStream on purpose, one to be able to inspect the messages logged by the logger instance created internally in the tested module and the other to create new fake console streams)

rpl

@saintsebastian here is a new round of review comments, they are mostly related to some fixes on the test cases (my apologies, I should have notice them in the previous review).

rpl · 2017-10-30T14:38:31Z

tests/unit/test.program.js

+      })
+      .then(makeSureItFails())
+      .catch((error) => {
+        if (!(error instanceof UsageError)) {


@saintsebastian This should be:

const {capturedMessages} = consoleStream; consoleStream.stopCapturing(); if (!(error instanceof UsageError)) { throw error; } assert.match(error.message, /some error/); ...

to be sure that the test still fails if an UsageError is received here.

rpl · 2017-10-30T14:42:53Z

tests/unit/test.program.js

+      })
+      .then(makeSureItFails())
+      .catch((error) => {
+        if (!(error instanceof UsageError)) {


same as the previous comment (at line 539)

saintsebastian · 2017-11-04T10:11:13Z

@rpl just to clarify, you would want shouldExitProgram: true and not to make any error assertions because of that? Maybe it makes sense to test both behaviors?

rpl · 2017-11-04T16:33:06Z

@saintsebastian no, actually you are right, we already have a test for that behavior at line 70 (it exits 1 on a thrown error), and so the only tweak needed here are the ones related to these two review comments #1080 (comment) and #1080 (comment)

saintsebastian · 2017-11-05T10:37:02Z

@rpl thanks for your reviews! hope this is ready for last reviews

coveralls · 2017-11-05T10:42:16Z

Coverage remained the same at 100.0% when pulling 737dd74 on saintsebastian:config-option into 10da8a7 on mozilla:master.

kumar303

Hi @saintsebastian , I'm excited about this patch! I have requested some changes but if you have already discussed them with @rpl just let me know what I missed.

Sorry for the delayed response. I've been busy preparing addons.mozilla.org for the Firefox Quantum launch but it's finally over!

kumar303 · 2017-11-14T22:26:36Z

src/config.js

@@ -50,7 +50,7 @@ export function applyConfigToArgv({
        `an unknown option: "${option}"`);
    }
    if (options[decamelizedOptName].type === undefined) {
-      throw new WebExtError(
+      throw new UsageError(


I think this should be a WebExtError because when an option is defined without a type it's a programming error within web-ext itself (option types are defined at the bottom of program.js). It's not due to incorrect usage and it's not something the developer would be able to fix.

@kumar303 ah, you are definitely right, and this change is my fault, it is related to this comment #1080 (comment) (Sorry @saintsebastian).

Nevertheless, to prevent this check from be read wrong in the future, how about adding an inline comment near this throw to briefly explain what this error is about?

Another interesting option (I can file a bug so that we can handle it in a follow up) would be to use a more specific type in the flow annotations here:

web-ext/src/program.js

Line 96 in a55ee4c

setGlobalOptions(options: Object): Program {

so that we can make flow to raise an error during its static analysis if any of the options defined in program.js is missing the type property.

@rpl good idea about defining the shape of options

Is this PR a good place to add that type? @kumar303 @rpl

@kumar303 @saintsebastian we could definitely define and add the additional flow types for the options in this PR, the amount of changes should not be that big, but it could be reasonable to handle it in as a separate issue and pull requests, so that we are free to discuss the details of it without blocking this PR.

In the meantime, I added an issue related to it as #1144 (which also contains some additional info about the change, e.g. where these new flow types would be used and what their flow type definitions could probably look like).

kumar303 · 2017-11-14T22:30:29Z

src/program.js

@@ -265,6 +286,13 @@ Example: $0 --help run.
      describe: 'Disable all features that require standard input',
      type: 'boolean',
    },
+    'config': {
+      alias: 'c',
+      describe: 'Location of the config file',


I think 'Path to the config file' would be a bit more explicit

kumar303 · 2017-11-14T22:31:18Z

src/util/logger.js

@@ -110,7 +110,7 @@ export type CreateBunyanLogParams = {|
 export type CreateBunyanLogFn = (params: CreateBunyanLogParams) => Logger;

 export type CreateLoggerOptions = {|
-  createBunyanLog: CreateBunyanLogFn,
+  createBunyanLog?: CreateBunyanLogFn,


Was this type change necessary?

kumar303 · 2017-11-14T22:37:56Z

tests/unit/test.config.js

-      }, WebExtError,
-        'WebExtError: Option: apiUrl was defined without a type.');
+      }, UsageError,
+        'UsageError: Option: apiUrl was defined without a type.');
    });

    it('throws an error when the type of one of them is in config' +


This test looks like a duplicate of https://github.com/saintsebastian/web-ext/blob/737dd7495f090e5334b9ccd7ee8ad9a809ef4031/tests/unit/test.config.js#L640 You can delete it. It looks like the intention of the second test was to see if the code still works when one of the options has a type and the other does not but I don't think that's a useful test.

kumar303 · 2017-11-14T23:00:12Z

tests/unit/test.program.js

+
+    consoleStream.stopCapturing();
+    consoleStream.flushCapturedLogs();
+    consoleStream.startCapturing();


This is interesting. I see what you're doing here but I'd like to see it moved only to a single test instead of multiple tests. The test should be an assertion of how Program() will log any UsageError to the console. Actually, I was surprised that we did not have test coverage of this already. Good catch.

After you add a single test for this logic, your assertion of the UsageError messages (like assert.match(error.message, /bad path/)) will be enough. This is because we will know a thrown UsageError will always be logged -- you don't have to test the console stream every time.

Also, you should add a comment around the stream capturing/re-capturing logic because it's not obvious that it is working around the test suite itself.

That makes a lot of sense. I was not sure what source of error should be, so I made command itself throw, let me know if it should be a dependency.

kumar303 · 2017-11-14T23:12:54Z

tests/unit/test.program.js

+        sinon.assert.calledWith(fakeLoadJSConfigFile,
+                                sinon.match(resolvedFakePath));
+        sinon.assert.calledOnce(fakeApplyConfigToArgv);
+        sinon.assert.calledWith(fakeApplyConfigToArgv, sinon.match({


I think it would be better to check all parameters for the call to fakeApplyConfigToArgv because Flow isn't going to catch all configuration problem, especially the options parameter.

I assume options will always be the same (set in program.js), but i added a check for argv.

kumar303 · 2017-11-14T23:14:02Z

tests/unit/test.program.js

+      });
+  });
+
+  it('throws a UsageError when the config file can\'t be loaded', () => {


I don't think you need this test because the behavior of handling thrown errors is already well defined.

I can see that perhaps you wanted to be sure that the config checking code is executed from within the catch block but I don't think it's necessary to cover that logic. If one day we encounter a bug where someone accidentally moved it outside of the catch block then we could add a test for it. Until then, I think code review is a better way to make sure the config logic is executed with the catch block.

kumar303 · 2017-11-14T23:15:18Z

tests/unit/test.program.js

+      }));
+  });
+
+  it('throws when config file loading raises an unexpected error', () => {


Similar to my last comment, I also don't think you need this test.

kumar303 · 2017-11-14T23:15:53Z

tests/unit/test.program.js

+      });
+  });
+
+  it('throws a UsageError when the loaded config can\'t be applied', () => {


Same here :) I say delete the test.

kumar303 · 2017-11-14T23:16:19Z

tests/unit/test.program.js

+      }));
+  });
+
+  it('throws when fakeApplyConfigToArgv throws an unexpected error', () => {


This can be deleted. I don't think it's helpful to check that an error is thrown.

coveralls · 2017-11-20T19:31:21Z

Coverage remained the same at 100.0% when pulling 67d04a8 on saintsebastian:config-option into a55ee4c on mozilla:master.

coveralls · 2017-11-20T19:31:44Z

Coverage remained the same at 100.0% when pulling 67d04a8 on saintsebastian:config-option into a55ee4c on mozilla:master.

saintsebastian · 2017-11-20T19:31:46Z

@rpl @kumar303 Thanks for the review! Let me know if something else is missing

coveralls · 2017-11-20T19:35:24Z

Coverage remained the same at 100.0% when pulling 67d04a8 on saintsebastian:config-option into a55ee4c on mozilla:master.

kumar303

The code and tests are looking good but things aren't quite hooked up right. Here's one change I needed but there are a couple other issues too. If you try out a config file with a couple different options (a global one plus a command option) you should find the issues.

You can ignore the logging statements:

diff --git a/src/config.js b/src/config.js
index edcf65b..c92ae28 100644
--- a/src/config.js
+++ b/src/config.js
@@ -86,6 +86,7 @@ export function applyConfigToArgv({
       continue;
     }
 
+    log.debug(`Setting config option ${option} : ${configObject[option]}`);
     newArgv[option] = configObject[option];
   }
   return newArgv;
diff --git a/src/program.js b/src/program.js
index d5143a0..5767f1f 100644
--- a/src/program.js
+++ b/src/program.js
@@ -155,18 +155,21 @@ export class Program {
         });
       }
 
+      let argvFromConfig = { ...argv };
       if (argv.config) {
         const configFileName = path.resolve(argv.config);
         const configObject = loadJSConfigFile(configFileName);
-        applyConfigToArgv({
+        log.debug('config from file', configObject);
+        argvFromConfig = applyConfigToArgv({
           argv,
           configFileName,
           configObject,
           options: this.options,
         });
+        log.debug('new argv', argvFromConfig);
       }
 
-      await runCommand(argv, {shouldExitProgram});
+      await runCommand(argvFromConfig, {shouldExitProgram});
 
     } catch (error) {
       if (!(error instanceof UsageError) || argv.verbose) {

kumar303 · 2017-12-05T16:57:35Z

tests/unit/test.program.js

+        consoleStream.stopCapturing();
+        assert.match(error.message, /some error/);
+        assert.ok(
+          capturedMessages.some((message) => message.match(/some error/)


This is a great test! Thanks

coveralls · 2017-12-10T14:34:50Z

Coverage remained the same at 100.0% when pulling 66e696c on saintsebastian:config-option into a55ee4c on mozilla:master.

saintsebastian · 2017-12-10T14:38:11Z

@kumar303 I fixed one broken test, but didn't notice any other errors so far, please let me know if you see any inconsistencies.

kumar303 · 2017-12-10T21:07:30Z

src/program.js

+      if (argv.config) {
+        const configFileName = path.resolve(argv.config);
+        const configObject = loadJSConfigFile(configFileName);
+        argvFromConfig = applyConfigToArgv({


Thanks for correcting this. It also needs test coverage. When I comment out your fix, the tests all pass -- at least one should fail.

kumar303 · 2017-12-10T21:53:29Z

How are you testing it out? I made a config file like this:

module.exports = {
  sourceDir: 'extension',
  run: {
    firefox: 'firefoxdeveloperedition',
  },
};

When I execute it inside sound-control, I hit a couple problems with the sourceDir option. My command:

web-ext run --verbose -c web-ext-config.js

For sanity, you can run the same thing without a config and watch it succeed:

web-ext run --verbose -s extension -f firefoxdeveloperedition

The first thing I am noticing is that option recursion is not implemented right. In this case, when {run: { firefox: {...}}} is parsed, the newArgv.sourceDir value gets erased. This patch fixes it but try to write a failing test before adding this patch:

diff --git a/src/config.js b/src/config.js
index edcf65b..4aecd70 100644
--- a/src/config.js
+++ b/src/config.js
@@ -36,7 +36,7 @@ export function applyConfigToArgv({
       typeof configObject[option] === 'object') {
       // Descend into the nested configuration for a sub-command.
       newArgv = applyConfigToArgv({
-        argv,
+        argv: newArgv,
         configObject: configObject[option],
         options: options[option],
         configFileName});

The next thing I noticed is that option coercion needs to be implemented. Take a look at the coerce callback for sourceDir. The config file needs to invoke these somehow, like:

diff --git a/src/config.js b/src/config.js
index edcf65b..701b27a 100644
--- a/src/config.js
+++ b/src/config.js
@@ -36,7 +36,7 @@ export function applyConfigToArgv({
       typeof configObject[option] === 'object') {
       // Descend into the nested configuration for a sub-command.
       newArgv = applyConfigToArgv({
-        argv,
+        argv: newArgv,
         configObject: configObject[option],
         options: options[option],
         configFileName});
@@ -87,6 +87,11 @@ export function applyConfigToArgv({
     }

     newArgv[option] = configObject[option];
+    const coerce = options[decamelizedOptName].coerce;
+    if (coerce) {
+      log.debug(`Calling coerce() on ${option}`);
+      newArgv[option] = coerce(newArgv[option]);
+    }
   }
   return newArgv;
 }

This patch would need test coverage too.

I hope that helps!

It was tricky to track this down. I had to add a breakpoint in config.js and run:

node --inspect --debug-brk `which web-ext` run --verbose -c web-ext-config.js

kumar303 · 2017-12-13T16:23:22Z

Hi @saintsebastian , good news! @rpl and I are at the Mozilla All-Hands right now (we are missing you). We decided that we have time to work on the config features so I'm going to land the patch and help with the fixes. Thanks so much for all of your work on this. Since you added the much needed glue to hook things up, it revealed a couple hidden bugs :)

saintsebastian · 2017-12-13T16:46:19Z

@kumar303 @rpl wish you a great time in Austin! Hope to join you next time:) Glad the config features are moving forward, and feel free to ping me.

feat: basic implementation of command line config option

5552269

saintsebastian force-pushed the config-option branch from c138bcc to 5552269 Compare September 18, 2017 11:26

rpl requested changes Sep 18, 2017

View reviewed changes

kumar303 mentioned this pull request Sep 19, 2017

Discover config files in multiple places #730

Closed

fix: catching config parsing errors

1c4ffd8

saintsebastian force-pushed the config-option branch from 295be34 to 1c4ffd8 Compare September 19, 2017 15:22

test: test for config option

8c363e1

saintsebastian force-pushed the config-option branch from 03c9a6c to 8c363e1 Compare September 20, 2017 19:07

rpl requested changes Sep 21, 2017

View reviewed changes

test: additional tests for config option

482fbb5

rpl requested changes Sep 28, 2017

View reviewed changes

kumar303 mentioned this pull request Oct 2, 2017

Ignore files based on VCS ignore files (like .gitignore) or using a custom ignore files (.wextignore) #940

Open

saintsebastian added 2 commits October 21, 2017 14:34

feat: add explicit type for defaultVersionGetter

5d1db6c

test: cleanup existing tests

67ea96f

test: testing logging for config loading using console stream

afe0454

fix: corrected types of errors thrown by applyConfigToArgv and tested…

1e8acb9

… them

rpl reviewed Oct 30, 2017

View reviewed changes

rpl requested changes Oct 30, 2017

View reviewed changes

test: throw for unexpected config error

737dd74

rpl approved these changes Nov 6, 2017

View reviewed changes

rpl requested a review from kumar303 November 6, 2017 13:04

kumar303 suggested changes Nov 14, 2017

View reviewed changes

saintsebastian force-pushed the config-option branch from e655ca4 to a81e071 Compare November 20, 2017 19:27

test: more specific testing

67d04a8

saintsebastian force-pushed the config-option branch from a81e071 to 67d04a8 Compare November 20, 2017 19:30

kumar303 self-requested a review November 27, 2017 23:41

kumar303 suggested changes Dec 5, 2017

View reviewed changes

rpl mentioned this pull request Dec 6, 2017

Add type checking for global / command options #1144

Open

fix: connect argv from config to command and test

66e696c

kumar303 reviewed Dec 10, 2017

View reviewed changes

kumar303 merged commit 8a8ee94 into mozilla:master Dec 13, 2017

saintsebastian deleted the config-option branch December 13, 2017 16:40

saintsebastian restored the config-option branch December 13, 2017 16:41

	if (options[decamelizedOptName].type === undefined) {
	throw new WebExtError(
	`Option: ${option} was defined without a type.`);
	}

	if (optionType !== expectedType) {
	throw new UsageError(`The config file at ${configFileName} specified ` +
	`the type of "${option}" incorrectly as "${optionType}"` +
	` (expected type: "${expectedType}")`);
	}

feat: basic implementation of command line config option #1080

feat: basic implementation of command line config option #1080

Conversation

saintsebastian commented Sep 16, 2017 • edited

coveralls commented Sep 18, 2017

rpl commented Sep 18, 2017

rpl Sep 18, 2017 • edited

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

kumar303 commented Sep 18, 2017 • edited

rpl commented Sep 18, 2017

saintsebastian commented Sep 19, 2017

rpl commented Sep 19, 2017

coveralls commented Sep 19, 2017

coveralls commented Sep 19, 2017

coveralls commented Sep 20, 2017

coveralls commented Sep 20, 2017

rpl 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

saintsebastian commented Sep 22, 2017

rpl 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

coveralls commented Oct 21, 2017

coveralls commented Oct 21, 2017

saintsebastian commented Oct 21, 2017 • edited

Choose a reason for hiding this comment

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintsebastian commented Nov 4, 2017 • edited

rpl commented Nov 4, 2017

saintsebastian commented Nov 5, 2017

coveralls commented Nov 5, 2017

kumar303 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

kumar303 Nov 14, 2017 • edited

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

kumar303 Nov 14, 2017 • edited

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

coveralls commented Nov 20, 2017

coveralls commented Nov 20, 2017

saintsebastian commented Nov 20, 2017

coveralls commented Nov 20, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 10, 2017

saintsebastian commented Dec 10, 2017

saintsebastian commented Sep 16, 2017 •

edited

rpl Sep 18, 2017 •

edited

kumar303 commented Sep 18, 2017 •

edited

saintsebastian commented Oct 21, 2017 •

edited

saintsebastian commented Nov 4, 2017 •

edited

kumar303 Nov 14, 2017 •

edited

kumar303 Nov 14, 2017 •

edited