Fixing: #604

[SatyamMattoo]

Short Description

Disables logging of env variables and populates the args according to their priority.

Related issue

Fixes #604

Implementation Details

Added a to function to populate the args in a different cli.ts file. Default values are not provided to the yargs but are added when the function returns the values. This also ensures the env variables are not logged on pnpm start --help .

QA Notes

I have added tests related to this function in cli.test.ts file. More tests can be added if needed.

Checklist before requesting a review

• I have performed a self-review of my code
• I have added unit tests
• Changesets have been added (if there are production code changes)

[josephjclark]

Looks good @SatyamMattoo! I appreciate the unit tests.

Before I test this locally I've left a few small comments and questions you might want to take a look at.

Thanks!

[josephjclark]

Why did you decide to remove all this from start.ts and add it to cli.ts?

[SatyamMattoo]

I was thinking it might be better to include all the types and functions related to argument parsing in the same file. If you see any issues with this approach, we can alternatively export them from the start.ts file to the cli.ts file.

Please let me know your thoughts.

[josephjclark]

hmm. I'm not sure. I can almost see it, but I think the breakdown of what goes where feels a bit muddy.

In other packages we use cli.ts as the entrypoint, which we don't do here.

I'll have a think about it and get back to you

[SatyamMattoo]

Sure sir, I've committed rest of the changes. I will be happy to make any additional change.

[josephjclark]

Thank you @SatyamMattoo

I've been busy here today so I'll have to get back to you next week. Hopefully we can wrap it up quickly from here!

[josephjclark]

Hey @SatyamMattoo, here's what we'll do here:

• Move cli.ts into util/
• Set parseArgs to be the default export of cli.ts. And maybe call the function cli (although a default function export doesn't need a name so I'd probably prefer it to be anonymous`
• In start.ts, update your import and call cli(process.argv) instead of pargsArgs(process.argv)
• Move your test file and update any tests that might need it

I do agree that it's a bit nicer to isolate the yargs stuff in one place. This approach keeps the cli stuff clean and discoverable, but de-prioritises its importance a little. This helps avoid the confusion with how other packages deal with cli.ts.

[SatyamMattoo]

Hey @josephjclark I have made the changes accordingly, you can review them at your convenience and let me know if there are any more changes needed.

Best regards.

[josephjclark]

@SatyamMattoo Thank you for this, it looks great. I'll merge it in and prepare it for release this week.

For future reference, please try to avoid commit messages like requested changes. You should pretend like I didn't ask for the changes and instead document (broadly) what you did.