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

Bug fix: create utility for dealing with help input #5871

Merged
merged 5 commits into from
Feb 10, 2023
Merged

Conversation

eggplantzzz
Copy link
Contributor

@eggplantzzz eggplantzzz commented Feb 6, 2023

PR description

addresses #2041

This PR creates a utility method for parsing help input. It then uses that same logic in console-child.js. This is so that we use the same logic for parsing help input from both the command line and the development console (repl).

I also found out that yargs uses console.error for the showHelp method which we use to display general help in Truffle. This was causing a bug where general help would not print in the development console as the stderr was not "hooked up". So this PR also passes the "log" argument to this method to force it to use console.log.

Testing instructions

To test this, checkout this branch and run the development console. Enter "help" and you can see that the general help is printed to the screen.

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.

@eggplantzzz eggplantzzz changed the title pull out utility for dealing with help input and force yargs to use c… Bug fix: create utility for dealing with help input Feb 6, 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.

Some small nits.

let displayHelp = false;
//User only enter truffle with no commands, let's show them what's available.
if (inputStrings.length === 0) {
displayHelp = true;
Copy link
Member

Choose a reason for hiding this comment

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

The code can return true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it can't, that does not conform to the method signature as I wrote it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh do you mean to return { displayHelp: true } in both of these cases?

Copy link
Member

@cds-amal cds-amal Feb 6, 2023

Choose a reason for hiding this comment

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

Yes, { displayHelp: true } in both of those cases.

Copy link
Contributor Author

@eggplantzzz eggplantzzz Feb 7, 2023

Choose a reason for hiding this comment

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

Sure but that is more a stylistic thing than anything.

Copy link
Member

@cds-amal cds-amal Feb 8, 2023

Choose a reason for hiding this comment

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

I consider it to be a readability refactor. The current iteration makes it easier to understand the success path of the function. It reduces the need to track displayHelp's state throughout the function, as well as clearly outlining the exceptional, failure, cases with the early returns.

Edit: I can see where it can come down to stylistic choices. At times, I like to initialize a result variable and change its state until returning it. This article nicely summarizes the return early pattern

packages/core/lib/cliHelp.js Outdated Show resolved Hide resolved
packages/core/lib/cliHelp.js Outdated Show resolved Hide resolved
packages/core/lib/command-utils.js Outdated Show resolved Hide resolved
packages/core/lib/command-utils.js Show resolved Hide resolved
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.

Looks good to me, pending tests, of course.

@eggplantzzz eggplantzzz merged commit d1af23f into develop Feb 10, 2023
@eggplantzzz eggplantzzz deleted the console-help branch February 10, 2023 18:23
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.

2 participants