Dropping args and replacing by minimist#350
Dropping args and replacing by minimist#350mcollina merged 2 commits intopinojs:masterfrom marcelfranca:master
Conversation
jsumners
left a comment
There was a problem hiding this comment.
I suspect there is more work that needs to be done in addition to changing the package.json.
|
I might understood wrong but I was told to create a DRAFT as soon as I started to work to show that someone is working on it. |
|
@jsumners we are used to opening Draft PRs early on, no need to request changes on a Draft PR anyway since it can't be merged. @marcelfranca you mentioned you're having troubles migrating some options from args to minimist, shout if you need any input so the people contributing to this repo can help out. |
|
In the future, please start work from a feature branch. See https://jrfom.com/posts/2017/03/08/a-primer-on-contributing-to-projects-with-git/#create-a-feature-branch |
jsumners
left a comment
There was a problem hiding this comment.
This PR results in a loss of CLI help. Prior to this PR, one could pino-pretty --help and get descriptions of each allowed option along with some helpful examples. With this PR, pino-pretty --help just hangs waiting for input.
If minimist does not support this feature, and newer versions of args have not resolved the problem prompting issue #336, then I recommend looking at sade instead -- https://github.com/lukeed/sade#single-command-mode
|
Yes, Also, I created a PR for Here is the link for the PR: |
You should add a test to that PR that fails prior to your fix. |
jsumners
left a comment
There was a problem hiding this comment.
I don't like moving the help text out to a text file:
- It will get out of sync
- The examples are no longer contextual, they are only available in the root
--help
But I'm not going to block this.
I understand that and I can agree. Unfortunately that's the only option with minimist (afaik), also used in https://github.com/fastify/fastify-cli/ |
Pull Request Test Coverage Report for Build 2542653509
💛 - Coveralls |
Pull Request Test Coverage Report for Build 2542653509
💛 - Coveralls |
Fix issue #336