Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dropping args and replacing by minimist #350

Merged
merged 2 commits into from
Jun 22, 2022
Merged

Dropping args and replacing by minimist #350

merged 2 commits into from
Jun 22, 2022

Conversation

marcelfranca
Copy link
Contributor

Fix issue #336

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

I suspect there is more work that needs to be done in addition to changing the package.json.

@marcelfranca
Copy link
Contributor Author

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.

@simoneb
Copy link
Contributor

simoneb commented Jun 15, 2022

@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.

@marcelfranca marcelfranca marked this pull request as ready for review June 16, 2022 16:47
@jsumners
Copy link
Member

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

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

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

@marcelfranca
Copy link
Contributor Author

Yes, minimist does support, I'll add it.

Also, I created a PR for args that fixes the problem we had with it, but no updates so far.

Here is the link for the PR:
leo/args#166

@jsumners
Copy link
Member

Also, I created a PR for args that fixes the problem we had with it, but no updates so far.

Here is the link for the PR: leo/args#166

You should add a test to that PR that fails prior to your fix.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

I don't like moving the help text out to a text file:

  1. It will get out of sync
  2. The examples are no longer contextual, they are only available in the root --help

But I'm not going to block this.

@jsumners jsumners dismissed their stale review June 22, 2022 11:44

Not blocking

@simoneb
Copy link
Contributor

simoneb commented Jun 22, 2022

I don't like moving the help text out to a text file:

  1. It will get out of sync
  2. 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/

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2542653509

  • 0 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2542630725: 0.0%
Covered Lines: 383
Relevant Lines: 383

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2542653509

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2542630725: 0.0%
Covered Lines: 383
Relevant Lines: 383

💛 - Coveralls

@mcollina mcollina merged commit 2928d13 into pinojs:master Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants