liquidapps-io / zeus-sdk

EOSIO & EVM smart contract unit testing suite and API provider
51 stars 24 forks source link

added rc file functionality and rc file ignore flag #9

Closed prcolaco closed 4 years ago

prcolaco commented 4 years ago

This PR includes:

handy to have custom defaults for zeus :)

NatPDeveloper commented 4 years ago

Created a PR for this internally, will update this PR once we've implemented, modified, or rejected the PR.

PRs are generated in our internal pipeline then pushed to this repo. As a result, this PR will be closed in the case of being implemented, modified, or rejected.

prcolaco commented 4 years ago

Sorry Nat but just found some problems with this PR that I am working on now... will let you know when it is corrected.

NatPDeveloper commented 4 years ago

I'm getting

Unknown arguments: deploy, canned-boxes

with:

node $PWD/zeus-cmd/dist/index.js deploy canned-boxes --no-release

Seems to be coming from:

if (!currentYargs.argv.rcIgnore && fs.existsSync(currentYargs.argv.rcFile)) {
  console.log(`loading options from rc file ${currentYargs.argv.rcFile}`);
  const config = JSON.parse(fs.readFileSync(currentYargs.argv.rcFile));
  currentYargs = currentYargs.config(config);
} else if (currentYargs.argv.rcIgnore) {
  console.log('ignoring rc file');
}

I added some logs:

console.log('!rcIgnore: ', !currentYargs.argv.rcIgnore)
console.log('fs.existsSync(currentYargs.argv.rcFile) ', fs.existsSync(currentYargs.argv.rcFile))
console.log('rcIgnore: ', currentYargs.argv.rcIgnore)

result:

!rcIgnore:  true
fs.existsSync(currentYargs.argv.rcFile)  false
rcIgnore:  false
prcolaco commented 4 years ago

that problem shouldn't be from this, at least I don't have it here... the problem that I am having is that the options from the RC file are not passed to other commands, like 'deploy'... for that command is as if the config file wasn't read at all :( there are other ways of implementing this in yargs, so I am experimenting now...

NatPDeveloper commented 4 years ago

The previous error is from our build pipeline.

It passes when the section in question is commented out, and it does not pass when it is commented in.

This is odd because neither option should be executed because the console logs confirm that you get a true and a false for the if, then the else if is also false.

Furthermore, I do not see either console log within the if nor the else if confirming it should not be reading that section.

If you're exploring new syntaxes, perhaps you can adopt something like our compile command:

https://github.com/liquidapps-io/zeus-sdk/blob/master/boxes/groups/core/build-extensions/extensions/commands/compile.js

prcolaco commented 4 years ago

Ok, I think it is now ok. Had to implement it in a yargs middleware, ~and had to define some kind of separation of concerns for sub-commands, so now the ~/.zeus/zeusrc.json file has a global key for options that apply to all sub-commands, then each sub-command can have their keys to describe each own options, and only the options that apply to current sub-command are loaded.~ EDIT: this last part is not needed, as I found another way to match valid entries in the rc file. An example of a zeusrc.json is like this:

{
    "verbose": true,
    "type": "local",
    "update-mapping": true,
    "test": true
}

It is kind of odd that the configuration file handling of yargs is so limited. But I couldn't find any other way besides implementing my own.

prcolaco commented 4 years ago

Please check the edits of my last comment... I consider this done. Please check. Now only the valid entries of the rc file to the current command will be applied, and rc is flat without the hierarchy that my last version had, so easier to write and understand. The rc file only supports kebab/snake case and translates to camel case keys as well. Note: in the rc alias should not be used, as they will not be able to match to their long option keys.

NatPDeveloper commented 4 years ago

Thanks for that. Updated docs and PR. Testing and reviewing internally.

NatPDeveloper commented 4 years ago

merged internally, thank you for the PR!