Skip to content

Conversation

@gagik
Copy link
Contributor

@gagik gagik commented Nov 27, 2025

This moves more of the logic into arg-parser and seperates CLI-repl-specific bits.
There are some further updates I'd apply afterwards in #2589 to make this reusable in MCP but this keeps changes minimal.

Copilot AI review requested due to automatic review settings November 27, 2025 11:56
@gagik gagik requested a review from a team as a code owner November 27, 2025 11:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the argument parsing logic by moving the core parseCliArgs functionality from cli-repl to the arg-parser package, improving code organization and separation of concerns. A new wrapper function parseMongoshCliArgs is introduced in cli-repl to handle CLI-specific error formatting.

Key changes:

  • Moved parseCliArgs, verifyCliArguments, getLocale, and related logic from cli-repl/arg-parser to arg-parser/arg-parser
  • Created parseMongoshCliArgs wrapper in cli-repl to handle UnknownCliArgumentError with CLI-specific formatting
  • Relocated test suite from cli-repl to arg-parser package
  • Moved yargs-parser dependency from cli-repl to arg-parser package

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/cli-repl/src/run.ts Updated import and function call to use parseMongoshCliArgs
packages/cli-repl/src/arg-parser.ts Replaced implementation with thin wrapper that catches UnknownCliArgumentError and formats CLI errors
packages/cli-repl/src/arg-parser.spec.ts Reduced to minimal tests verifying the wrapper functionality
packages/cli-repl/package.json Removed yargs-parser dependency
packages/arg-parser/src/index.ts Exported parseCliArgs and UnknownCliArgumentError
packages/arg-parser/src/arg-parser.ts Added core argument parsing implementation moved from cli-repl
packages/arg-parser/src/arg-parser.spec.ts Added comprehensive test suite moved from cli-repl
packages/arg-parser/package.json Added yargs-parser dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@addaleax addaleax added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Nov 27, 2025
Copy link
Collaborator

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Hm yeah, the shell-api complaint is a fair one – I think a relatively easy way out of this would be to put make @mongosh/arg-parser/arg-parser a separate export in package.json instead of including it in its index.ts?

@gagik gagik changed the title chore: move parseCliArgs to args-parser chore: move parseCliArgs to args-parser MCP-298 Nov 27, 2025
@gagik
Copy link
Contributor Author

gagik commented Nov 27, 2025

Hm yeah, the shell-api complaint is a fair one – I think a relatively easy way out of this would be to put make @mongosh/arg-parser/arg-parser a separate export in package.json instead of including it in its index.ts?

Running into module resolution issues because mongosh has "moduleResolution": "node", 😞

@addaleax
Copy link
Collaborator

Hm yeah, the shell-api complaint is a fair one – I think a relatively easy way out of this would be to put make @mongosh/arg-parser/arg-parser a separate export in package.json instead of including it in its index.ts?

Running into module resolution issues because mongosh has "moduleResolution": "node", 😞

We can and should feel free to change our moduleResolution value at this point 🙂

@gagik gagik force-pushed the gagik/move-arg-parser branch from 8a0c0c9 to 132a48d Compare November 28, 2025 13:00
@gagik gagik force-pushed the gagik/move-arg-parser branch from 8530957 to 0d5aeb5 Compare November 28, 2025 13:54
@gagik gagik merged commit da26658 into main Nov 28, 2025
155 checks passed
@gagik gagik deleted the gagik/move-arg-parser branch November 28, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants