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

Autocomplete details #267

Merged
merged 4 commits into from
Feb 22, 2021
Merged

Autocomplete details #267

merged 4 commits into from
Feb 22, 2021

Conversation

mmarcon
Copy link
Member

@mmarcon mmarcon commented Feb 22, 2021

Given that we are pulling the autocomplete symbols from the shell API, we can pull the documentation details from there too.

Feb-22-2021 08-19-58

Description

This change pulls in the shell's i18n package and uses that to enrich the shell CompletionItems with an example and a one-sentence explanation of what the command does. Unfortunately it does not look like the CompletionItem API supports links.

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

That string that is passed to Translator.translate() is overly verbose but there does not seem to be a way around that.

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@mmarcon
Copy link
Member Author

mmarcon commented Feb 22, 2021

As a side note, if we had a flag for deprecated methods that the extension can read when it loads the shell API symbols, we could tag those CompletionItems as deprecated and I believe VS Code will then render them with a strikethrough font.

This way, it does not crash.
Copy link

@rose-m rose-m left a comment

Choose a reason for hiding this comment

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

Awesome!! 😍 LGTM - but I'll let the others give their full approval!

@mcasimir
Copy link
Contributor

Wow 😄!

Would it be possible to move this to be part of the interface of the shell autocompleter? Is some change more, but if this is not urgent it would be good so we don't leak shell internals around and we'd get the same in all the shells.

@mcasimir
Copy link
Contributor

mcasimir commented Feb 22, 2021

Never mind, forgot that the shell auto-completer works in a different way, maybe when we put them back together.

Copy link
Contributor

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

@gribnoysup
Copy link
Contributor

RE failing to update mongosh deps to latest, it's probably related to #219 and for sure a few other things, there were a bunch of changes in latest mongosh and we definitely should take some time and update vscode to latest

@mmarcon
Copy link
Member Author

mmarcon commented Feb 22, 2021

image

@mmarcon mmarcon merged commit 4655b8c into master Feb 22, 2021
@mmarcon mmarcon deleted the autocomplete-details branch February 22, 2021 12:25
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.

4 participants