Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Enhancement: More robustly handle help requests in Truffle #5706

Merged
merged 24 commits into from Jan 11, 2023

Conversation

lsqproduction
Copy link
Contributor

@lsqproduction lsqproduction commented Nov 15, 2022

PR description

This addresses #5662.
Currently, We have help as a truffle command. And since our truffle commands were built using Yargs, we also have yargs default help info display via truffle <cmd> help or truffle <cmd> --help.
Truffle's help command uses the command's meta.help object and Yargs help uses the command's meta.builder object. It's often the info in these two objects are miss matching causing the help outputs to be different.

Should we update the documentation to let users know about the other syntax for getting help?

For this, I explored two options and decided to go with option 1.

  1. Turn off yargs's default for help. Use our own displayCommandHelp when --help is used anywhere in the command line.

Here are the reasons why I chose this option.
At this point, the info in the Help objects is more complete than in the Build object.
The output can be more customized.

Modify the inputStrings if they have --help in the command line before running through the command utils. As well as truffle --help <cmd>
I would love to know if there's a better way to handle this.
Added scenario testing of Help. Let me know if I'm missing any cases.
Do note, I did not include any fixes in this PR for getting help in truffle console or truffle develop.

  1. Keep the current yargs default, and edit our Help command's run.js to use yargs's default showHelp() for the particular command.

There are still some limitations to using Yargs for this.
Need to inject the command's options as well as global options.
Not enough control over how the format of the output will look.

Testing instructions

truffle help <cmd> & truffle <cmd> --help & truffle --help <cmd> should have the same output.
run the help scenario test.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

  • I have added any needed breaking-change and new-feature labels for the appropriate packages.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Thanks, @lsqproduction. It looks good, though I have a request to make the help system more inclusive to different invocations.

packages/core/cli.js Outdated Show resolved Hide resolved
@gnidan
Copy link
Contributor

gnidan commented Nov 15, 2022

Thanks for taking this on!

Copy link
Contributor

@dongmingh dongmingh left a comment

Choose a reason for hiding this comment

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

I ran the following test:
truffle help db
truffle --help db
truffle db help
truffle db --help
and they all give the same result:

  Usage:        truffle db <sub-command> [options]
  Available sub-commands: 
                serve 	Start the GraphQL server 
  Description:  Database interface commands

And
truffle help
truffle --help
also give the same results:

Truffle v5.6.5 - a development framework for Ethereum

Usage: truffle <command> [options]

Commands:
  cli.js build      Execute build pipeline (if configuration present)
  cli.js compile    Compile contract source files
  cli.js config     Set user-level configuration options
  cli.js console    Run a console with contract abstractions and commands
                    available
  cli.js create     Helper to create new contracts, migrations and tests
  cli.js dashboard  Start Truffle Dashboard to sign development transactions
                    using browser wallet
  cli.js db         Database interface commands
  cli.js debug      Interactively debug any transaction on the blockchain
  cli.js deploy     (alias for migrate)
  cli.js develop    Open a console with a local development blockchain
  cli.js exec       Execute a JS module within this Truffle environment
  cli.js help       List all commands or provide information about a specific
                    command
  cli.js init       Initialize new and empty Ethereum project
  cli.js migrate    Run migrations to deploy contracts
  cli.js networks   Show addresses for deployed contracts on each network
  cli.js obtain     Fetch and cache a specified compiler
  cli.js opcode     Print the compiled opcodes for a given contract
  cli.js preserve   Save data to decentralized storage platforms like IPFS and
                    Filecoin
  cli.js run        Run a third-party command
  cli.js test       Run JavaScript and Solidity tests
  cli.js unbox      Download a Truffle Box, a pre-built Truffle project
  cli.js version    Show version number and exit
  cli.js watch      Watch filesystem for changes and rebuild the project
                    automatically

Options:
  --help     Show help                                                 [boolean]
  --version  Show version number                                       [boolean]

Then I ran the same commands in truffle develop:

truffle(develop)> truffle db help
"ℹ️ : 'db' is not allowed within Truffle REPL"
truffle(develop)> truffle db --help
"ℹ️ : 'db' is not allowed within Truffle REPL"
truffle(develop)> truffle --help db
"ℹ️ : '--help' is not a valid Truffle command"

only truffle help db works properly:

truffle(develop)> truffle help db

  Usage:        truffle db <sub-command> [options]
  Available sub-commands: 
                serve 	Start the GraphQL server 
  Description:  Database interface commands

and truffle --help:

truffle(develop)> truffle --help
"ℹ️ : '--help' is not a valid Truffle command"

And, commands
truffle help
help
fail in truffle develop.

truffle(develop)> truffle help
TypeError: Cannot read properties of undefined (reading '#<Object>')
    at module.exports (/Users/dongminghwang/work/trufflesuite/truffle/packages/core/lib/commands/help/displayCommandHelp.js:9:46)
    at Object.module.exports [as run] (/Users/dongminghwang/work/trufflesuite/truffle/packages/core/lib/commands/help/run.js:5:11)
    at runCommand (/Users/dongminghwang/work/trufflesuite/truffle/packages/core/lib/command-utils.js:203:24)
truffle(develop)> help
TypeError: Cannot read properties of undefined (reading '#<Object>')
    at module.exports (/Users/dongminghwang/work/trufflesuite/truffle/packages/core/lib/commands/help/displayCommandHelp.js:9:46)
    at Object.module.exports [as run] (/Users/dongminghwang/work/trufflesuite/truffle/packages/core/lib/commands/help/run.js:5:11)
    at runCommand (/Users/dongminghwang/work/trufflesuite/truffle/packages/core/lib/command-utils.js:203:24)

Both truffle help help and help db work in truffle develop. If a subject, such as db, help, is required, we need to tell user about that.

@lsqproduction
Copy link
Contributor Author

Thank you, @dongmingh for the review. You're right. Help in the truffle repls has been broken for a while too. I want to save that work for another PR if possible targeting #5312 and #2041.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Thanks, @lsqproduction. This PR should have tests to cover these new help cases. While manually testing, I noticed an error: Not enough non-option arguments: got 0, need at least 1.

test output

$ truffle db --help serve
cli.js db

Database interface commands

Commands:
  cli.js db serve  Start Truffle's GraphQL UI playground

Not enough non-option arguments: got 0, need at least 1

packages/core/cli.js Outdated Show resolved Hide resolved
packages/core/cli.js Outdated Show resolved Hide resolved
@eggplantzzz
Copy link
Contributor

Note that we still need to maintain two versions of the options: the builder object and the options one.

Maybe it is worth rewriting the help so that all of the options are defined in terms of the information in the builder object? Does that make sense? Or rather maybe the help should get its information from the builder object instead of looking at options. If you are interested in something like this then ping me later and we can talk about it!

packages/core/cli.js Outdated Show resolved Hide resolved
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Hey @lsqproduction, It seems you merged develop into your branch. Can you rebase instead? I'm having a difficult time separating your commits while testing/reviewing.

This is what the commit log looks like
$ git log -40
Wed Dec 21 08:49:04 2022 -0500 32fa6b9f8 (HEAD -> betterHelp, origin/betterHelp) Merge branch 'develop' into betterHelp  [Liang Liang]
Sat Dec 17 18:52:45 2022 -0500 13280551b Merge pull request #5794 from trufflesuite/bump-w3  [tyler]
Fri Dec 16 15:37:55 2022 -0500 6a2316ec4 update expected message when connection to node fails for tests in provider  [eggplantzzz]
Fri Dec 16 14:40:43 2022 -0500 743259d65 fix some tests to deal with wonky type issues  [eggplantzzz]
Fri Dec 16 12:57:58 2022 -0500 1a8f1468c Merge branch 'develop' into betterHelp  [Liang Liang]
Fri Dec 16 12:01:34 2022 -0500 c1d71221c Merge pull request #5791 from trufflesuite/trufflemediate  [Harry Altman]
Fri Dec 16 11:25:47 2022 -0500 95c32972b bump web3 libs to 1.8.1  [eggplantzzz]
Thu Dec 15 18:40:34 2022 -0500 3d1c80d32 Update Ganache to 7.6.0  [Harry Altman]
Thu Dec 15 16:52:46 2022 -0500 c984fe37b (tag: v5.7.0, tag: truffle@5.7.0, tag: @truffle/workflow-compile@4.0.44, tag: @truffle/test@0.1.5, tag: @truffle/resolver@9.0.26, tag: @truffle/require@2.1.16, tag: @truffle/provisioner@0.2.69, tag: @truffle/provider@0.3.0, tag: @truffle/profiler@0.1.38, tag: @truffle/migrate@3.3.21, tag: @truffle/from-hardhat@0.1.14, tag: @truffle/fetch-and-compile@0.5.29, tag: @truffle/external-compile@2.0.65, tag: @truffle/environment@0.2.133, tag: @truffle/encoder@1.0.0, tag: @truffle/deployer@3.3.0, tag: @truffle/decoder@5.3.3, tag: @truffle/debugger@11.0.20, tag: @truffle/db@2.0.9, tag: @truffle/db-loader@0.2.9, tag: @truffle/db-kit@0.2.9, tag: @truffle/dashboard@0.3.3, tag: @truffle/dashboard-message-bus-e2e-test@0.1.13, tag: @truffle/core@5.7.0, tag: @truffle/contract-tests@0.1.116, tag: @truffle/config@1.3.46, tag: @truffle/compile-vyper@3.1.88, tag: @truffle/compile-solidity@6.0.53, tag: @truffle/compile-solidity-tests@0.1.12, tag: @truffle/box@2.1.66) Publish  [eggplantzzz]
Thu Dec 15 16:31:47 2022 -0500 b278409c6 Merge pull request #5788 from trufflesuite/console-fix  [tyler]
Thu Dec 15 15:42:21 2022 -0500 7f833fd61 optimize logic  [eggplantzzz]
Thu Dec 15 13:31:50 2022 -0500 313164251 add condition checking for 1 source to determine whether to filter artifact  [eggplantzzz]
Thu Dec 15 12:24:46 2022 -0500 8ebc4668a don't load Truffle's console contract into the console environment  [eggplantzzz]
Thu Dec 15 10:18:01 2022 -0500 ac013dc46 Merge pull request #5785 from trufflesuite/fix-migrate  [tyler]
Wed Dec 14 23:14:02 2022 -0700 86b0e3d40 Merge pull request #5743 from trufflesuite/implement-1193-provider  [Sukanya Parashar]
Wed Dec 14 17:47:55 2022 -0500 f83cf2de7 core(console): add workaround for weird migration format  [cds-amal]
Wed Dec 14 15:11:11 2022 -0700 b50b9e5fa Use the word collision instead of race condition in the note  [Sukanya Parashar]
Wed Dec 14 15:12:47 2022 -0500 daa121c24 refactor logic to process help input  [liang liang]
Wed Dec 14 12:27:52 2022 -0700 9b2b9142d Add note for the requestId counter  [Sukanya Parashar]
Wed Nov 30 14:17:26 2022 -0500 28dcc7a72 add yarn.lock  [liang liang]
Wed Nov 30 14:07:27 2022 -0500 827964ea7 add missing deps to @core/package.json  [liang liang]
Wed Nov 30 12:14:58 2022 -0500 d3719e8df Move help inputStrings process to command-utils  [liang liang]
Wed Nov 30 11:07:42 2022 -0500 ea6b337b2 Add error handling when subCommand is invalid  [liang liang]
Wed Nov 30 11:02:57 2022 -0500 a7d674d9d Add handling when help cmd is in wrong place  [liang liang]
Wed Nov 30 11:00:34 2022 -0500 762744819 Add scenario test for Help cmd  [liang liang]
Tue Nov 29 14:14:37 2022 -0500 21002466d Add complete Help command usage info  [liang liang]
Tue Nov 29 10:06:10 2022 -0500 735b3be51 improve code comment readability  [liang liang]
Tue Nov 15 18:10:10 2022 -0500 2a30ac65a add handling for `truffle --help <cmd>`  [liang liang]
Tue Nov 15 10:56:19 2022 -0500 c90016be2 change inputStrings when help or --help is present  [liang liang]
Tue Nov 15 10:35:44 2022 -0500 891a309de turn off yargs default behavior for truffle cmd help  [liang liang]
Wed Dec 14 13:23:36 2022 -0500 3123c7b87 Implement console.log in Truffle (#5687)  [cds-amal]
Tue Dec 13 15:46:46 2022 -0500 bcbc02658 Merge pull request #5780 from trufflesuite/remove-invalidation  [tyler]
Tue Dec 13 14:52:49 2022 -0500 d9457529c Merge pull request #5782 from trufflesuite/fix/in-test-runnable  [g. nicholas d'andrea]
Tue Dec 13 13:25:02 2022 -0500 7899cad72 Merge pull request #5763 from trufflesuite/sneved  [Harry Altman]
Tue Dec 13 12:55:18 2022 -0500 077ca84d7 (fix/in-test-runnable) Slightly improve type  [g. nicholas d'andrea]
Mon Dec 12 20:05:46 2022 -0500 f93ca58d6 Switch Test's `mochaRunner` to be a Promise  [g. nicholas d'andrea]
Mon Dec 12 17:15:07 2022 -0500 85e682fe8 Simplify transfers of names  [Harry Altman]
Mon Dec 12 16:00:46 2022 -0500 d5b0970be add debug to config  [eggplantzzz]
Mon Dec 12 13:52:26 2022 -0500 972a5d91f move before setup into beforeEach for config test  [eggplantzzz]
Mon Dec 12 13:35:52 2022 -0500 0a818bab0 add some debug logging to config  [eggplantzzz]%

packages/core/cli.js Outdated Show resolved Hide resolved
@lsqproduction lsqproduction changed the title Bug Fix: Produce same output fortruffle help <cmd> & truffle <cmd> help || --help Bug Fix: Handle --help as the help command Jan 9, 2023
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

truffle db help could error better. See: https://github.com/trufflesuite/truffle/blob/develop/packages/core/lib/commands/db/run.js#L16

Maybe change it to Unknown truffle db command: ...

@cds-amal cds-amal dismissed their stale review January 10, 2023 01:32

Changes were addressed.

Copy link
Contributor

@eggplantzzz eggplantzzz left a comment

Choose a reason for hiding this comment

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

i like it!

@eggplantzzz eggplantzzz changed the title Bug Fix: Handle --help as the help command Bug Fix: More robustly handle help requests in Truffle Jan 10, 2023
@eggplantzzz eggplantzzz changed the title Bug Fix: More robustly handle help requests in Truffle Enhancement: More robustly handle help requests in Truffle Jan 10, 2023
@sukanyaparashar
Copy link
Contributor

sukanyaparashar commented Jan 10, 2023

As mentioned in the testing instructions, should we have the same output for truffle <cmd> help and truffle help <cmd>? When I ran truffle help migrate, it shows the help for the "migrate" command but truffle migrate help doesn't show the help, instead it does the migration. Am I missing something here?
And this is the same result for "console" command too.

@lsqproduction
Copy link
Contributor Author

testing instructions

Apologizes, @sukanyaparashar. I forgot to edit the test instructions.
We decided to change the requirement. Now, help as the commend will only work in the first position. for example:truffle help compile. User can use the flag --help anywhere to get help info.

@sukanyaparashar
Copy link
Contributor

testing instructions

Apologizes, @sukanyaparashar. I forgot to edit the test instructions. We decided to change the requirement. Now, help as the commend will only work in the first position. for example:truffle help compile. User can use the flag --help anywhere to get help info.

Okay, that makes sense. Thanks @lsqproduction !

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Looks good @lsqproduction. Nice work getting this one across the finish line.

Comment on lines +56 to +76
it("displays help for given command when `--help` is at final position of the command line", async function () {
await CommandRunner.run("compile --help", config);
const output = logger.contents();
assert(output.includes("Description: Compile contract source files"));
}).timeout(20000);

it("displays help for given command when `--help` is at first position of the command line", async function () {
await CommandRunner.run("--help db serve", config);
const output = logger.contents();
assert(
output.includes("Description: Start Truffle's GraphQL UI playground")
);
}).timeout(20000);

it("displays help for given command when `--help` is in the middle of the command line", async function () {
await CommandRunner.run("db --help serve", config);
const output = logger.contents();
assert(
output.includes("Description: Start Truffle's GraphQL UI playground")
);
}).timeout(20000);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a change request. Just pointing out a unit test could validate the various --help positions, leaving only testing truffle help for integration tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm gonna come ask you about integration testing.

@lsqproduction lsqproduction merged commit 37db8eb into develop Jan 11, 2023
@lsqproduction lsqproduction deleted the betterHelp branch January 11, 2023 03:24
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.

None yet

6 participants