Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Fixed bin/rlp CLI command bug #60

Merged
merged 1 commit into from
Dec 20, 2018
Merged

Fixed bin/rlp CLI command bug #60

merged 1 commit into from
Dec 20, 2018

Conversation

holgerd77
Copy link
Member

No description provided.

@holgerd77 holgerd77 force-pushed the fix-cli-command-issue branch from 81a9ac2 to 75fb081 Compare December 20, 2018 10:29
@holgerd77
Copy link
Member Author

Have for now replaced the usage of util.promisify() in the new CLI command integration test with the callback version, since this was breaking on Node 6. However this raises the general question, if setup generally can be improved to also transpile tests and then run these transpiled tests so that we can independently from the targeted Node distribution version can use modern JS in the test setup.


async function bin() {
const { stdout, stderr } = await execP('./bin/rlp encode "[ 5 ]"')
exec('./bin/rlp encode "[ 5 ]"', (_error, stdout, _stderr) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I was running into linting errors like 'error' is declared but its value is never read here, however I didn't want to explicitly handle error and stuff here, so I googled towards the underscore solution. Is this a good way to do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I do this like this myself. Furthermore: _stderr could be deleted entirely since it's the last parameter.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.991% when pulling 75fb081 on fix-cli-command-issue into beddc69 on master.

@krzkaczor
Copy link
Contributor

However this raises the general question, if setup generally can be improved to also transpile tests and then run these transpiled tests so that we can independently from the targeted Node distribution version can use modern JS in the test setup.

Yes we can! (but that woudln't work anyway because we will transpile language features not node API). But bascially we can create tsconfig (like tsconfig.test.json targeting whatever node 6 supports and use to exectute with mocha (using TS_NODE_PROJECT ).

Buuuut when I think about it we already did language transpilation since we target ES5 and tests failed due to different node API. We could change lib (and possible @types/node version) settings to find these mistakes automatically.

@holgerd77
Copy link
Member Author

Ah, makes very much sense, for the moment maybe no need to do imminent changes in the setup.

Thanks for approval!

@holgerd77 holgerd77 merged commit ee0b433 into master Dec 20, 2018
@holgerd77 holgerd77 deleted the fix-cli-command-issue branch December 20, 2018 12:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants