Skip to content

Conversation

addaleax
Copy link
Collaborator

(includes #887 and commits from main for passing CI/avoiding conflicts, only the last commit is original here)

  • If the first positional argument ends in .mongo or .js and is not a
    connection string, treat it as a filename.
  • Add a --file (-f) flag for specifying filenames that avoids this
    ambiguity.
  • Internally, avoid referring to positional arguments in general and
    be specific about whether the connection specifier or the filenames
    are being accessed
  • As drive-by fixes, fix the spelling of tlsAllowInvalidHostname and
    drop the extra unnecessary start argument handling.

…OSH-769

- If the first positional argument ends in .mongo or .js and is not a
  connection string, treat it as a filename.
- Add a `--file` (`-f`) flag for specifying filenames that avoids this
  ambiguity.
- Internally, avoid referring to positional arguments in general and
  be specific about whether the connection specifier or the filenames
  are being accessed
- As drive-by fixes, fix the spelling of `tlsAllowInvalidHostname` and
  drop the extra unnecessary `start` argument handling.
Copy link
Collaborator

@mcasimir mcasimir left a comment

Choose a reason for hiding this comment

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

Nice! Requesting the change for the extension name .. unless i'm missing something :), consider it approved otherwise.

Do we want to output also a deprecation notice for positional files?

@addaleax
Copy link
Collaborator Author

Do we want to output also a deprecation notice for positional files?

I’m not sure, I think this would be a fairly frequently used feature and it might make sense to change the documentation first (and show a deprecation notice there).

Comment on lines +156 to +160
if (arg.startsWith('-')) {
throw new Error(
` ${clr(i18n.__(UNKNOWN), ['red', 'bold'])} ${clr(String(arg), 'bold')}
${USAGE}`
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really related to this change, but I was figuring out what case this is handling and found an interesting uhhh... bug? related to how yargs parser works I guess

This exists with unknown option error

mongosh --------foobar

This doesn't exit, the option is parsed by yargs to --------help: true so no help is printed

mongosh ----------help

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That”s … interesting, yeah. I feel like this might even qualify as yargs bug

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, true, I'll take a look if they have something like this already submitted and will open an issue/send a patch or something. I think it's a big enough corner case for us to not really worry about 🤔

Copy link
Collaborator

@gribnoysup gribnoysup 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!

Comment on lines +169 to +171
// All positional arguments are either in connectionSpecifier or fileNames,
// and should only be accessed that way now.
delete parsed._;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Introducing this legacy shell behavior was worth it if only for this change, makes it nice and explicit 👏

@addaleax addaleax merged commit 7517f44 into main May 20, 2021
@addaleax addaleax deleted the 769-dev branch May 20, 2021 14:50
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.

3 participants