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.

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

Refactor ParamedicConfig #169

Open
janpio opened this issue Jun 29, 2019 · 1 comment
Open

Refactor ParamedicConfig #169

janpio opened this issue Jun 29, 2019 · 1 comment

Comments

@janpio
Copy link
Member

janpio commented Jun 29, 2019

Currently the paramedic config is built via:

cordova-paramedic/main.js

Lines 78 to 206 in 5b8d306

var pathToParamedicConfig = util.getConfigPath(argv.config);
if (argv.version) {
console.log(require('./package.json')['version']);
process.exit(0);
} else if (pathToParamedicConfig || // --config
(argv.platform && argv.plugin)) { // or --platform and --plugin
var paramedicConfig = pathToParamedicConfig ?
ParamedicConfig.parseFromFile(pathToParamedicConfig) :
ParamedicConfig.parseFromArguments(argv);
// What and how to build and test
if (argv.platform) {
paramedicConfig.setPlatform(argv.platform);
}
if (argv.plugin) {
paramedicConfig.setPlugins(argv.plugin);
}
if (argv.cli) {
paramedicConfig.setCli(argv.cli);
}
if (argv.justBuild || argv.justbuild) {
paramedicConfig.setAction('build');
}
if (argv.action) {
paramedicConfig.setAction(argv.action);
}
if (argv.skipMainTests) {
paramedicConfig.setSkipMainTests(argv.skipMainTests);
}
if (argv.skipAppiumTests) {
paramedicConfig.setSkipAppiumTests(argv.skipAppiumTests);
}
if (argv.args) {
paramedicConfig.setArgs(argv.args);
}
// Emulator/Device to use for tests
if (argv.target) {
paramedicConfig.setTarget(argv.target);
}
// Test Result Server
if (argv.useTunnel) {
if (argv.useTunnel === 'false') {
argv.useTunnel = false;
}
paramedicConfig.setUseTunnel(argv.useTunnel);
}
// externalServerUrl?
// port?
// Test configuration
// timeout?
if (argv.outputDir) {
paramedicConfig.setOutputDir(argv.outputDir);
}
// cleanUpAfterRun?
if (argv.logMins) {
paramedicConfig.setLogMins(argv.logMins);
}
if (argv.tccDb) {
paramedicConfig.setTccDb(argv.tccDb);
}
// Sauce Labs
if (argv.shouldUseSauce) {
if (argv.shouldUseSauce === 'false') {
argv.shouldUseSauce = false;
}
paramedicConfig.setShouldUseSauce(argv.shouldUseSauce);
}
if (argv.sauceUser) {
paramedicConfig.setSauceUser(argv.sauceUser);
}
if (argv.sauceKey) {
paramedicConfig.setSauceKey(argv.sauceKey);
}
if (argv.buildName) {
paramedicConfig.setBuildName(argv.buildName);
}
if (argv.sauceDeviceName) {
paramedicConfig.setSauceDeviceName(argv.sauceDeviceName);
}
if (argv.saucePlatformVersion) {
paramedicConfig.setSaucePlatformVersion(argv.saucePlatformVersion);
}
if (argv.sauceAppiumVersion) {
paramedicConfig.setSauceAppiumVersion(argv.sauceAppiumVersion);
}
if (argv.sauceTunnelId) {
paramedicConfig.setSauceTunnelId(argv.sauceTunnelId);
}
// Others
if (argv.ci) {
paramedicConfig.setCI(argv.ci);
}
if (argv.fileTransferServer) {
paramedicConfig.setFileTransferServer(argv.fileTransferServer);
}

and
ParamedicConfig.parseFromArguments = function (argv) {
return new ParamedicConfig({
// What and how to build and test
platform: argv.platform,
plugins: Array.isArray(argv.plugin) ? argv.plugin : [argv.plugin],
verbose: !!argv.verbose,
cli: argv.cli,
action: argv.justbuild || argv.justBuild ? 'build' : 'run', // TODO argv.action
skipMainTests: argv.skipMainTests,
skipAppiumTests: argv.skipAppium,
args: '',
// Emulator/Device to use for tests
target: argv.target,
// Test Result Server
useTunnel: !!argv.useTunnel,
externalServerUrl: argv.externalServerUrl,
startPort: argv.startport || argv.port,
endPort: argv.endport || argv.port,
// Test configuration
// timeout?
outputDir: argv.outputDir ? argv.outputDir : null,
cleanUpAfterRun: !!argv.cleanUpAfterRun,
logMins: argv.logMins ? argv.logMins : null,
tccDb: argv.tccDbPath ? argv.tccDb : null,
// Sauce Labs
shouldUseSauce: !!argv.shouldUseSauce || false,
sauceUser: argv.sauceUser,
sauceKey: argv.sauceKey,
buildName: argv.buildName,
sauceDeviceName: argv.sauceDeviceName && argv.sauceDeviceName.toString(),
saucePlatformVersion: argv.saucePlatformVersion && argv.saucePlatformVersion.toString(),
sauceAppiumVersion: argv.sauceAppiumVersion && argv.sauceAppiumVersion.toString(),
sauceTunnelId: argv.sauceTunnelId,
// Others
ci: argv.ci,
fileTransferServer: argv.fileTransferServer
});
};
ParamedicConfig.parseFromFile = function (paramedicConfigPath) {
return new ParamedicConfig(require(paramedicConfigPath));
};

This is probably buggy, as if there is a config file, the defaults from parseFromArguments are not applied. Also the manual setting of config values in main.js is missing some parameters etc.

A better way would be to replace everything in main.js with e.g. var paramedicConfig = new ParamedicConfig(argv, pathToParamedicConfig); and then do all the logic in the constructor: Read and apply the file if supplied, then merge over all the manual arguments that are present in argv.

Pseudocode from @erisu:

constructor (args, file) {
        // defaults
        this._config = {
            add: 'my',
            required: 'default',
            values: 'here'
        };

        // apply file next
        this._config = Object.assign(this._config, file || {});

        // then args
        this._config = Object.assign(this._config, args || {});
    }
@erisu
Copy link
Member

erisu commented Jun 29, 2019

I believe all the configs are 1-level depth.

If there are any cases where it is not, Object.assign wont be able to handle this. You will need to do a deep merge.

Here is sample code I use in cordova-electron to handle deep merging.

https://github.com/apache/cordova-electron/blob/de08c6b50d6d11a002984abb1438991c648f2737/bin/templates/cordova/lib/util.js#L20-L32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants