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

Enable tab-completion for nuget package versions #42349

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Jul 24, 2024

The tab completion takes into account any fragments of the version entered, as well as the existing --prerelease flag to filter the returned versions.

Demo:

  • tab completion of package id based on fragment
  • --version completion of full id
  • --version completion using --prerelease to allow preview versions
  • --version completion using version fragments to narrow versions

name-completion

Cases to cover:

  • slow feeds - need some kind of max timeout to prevent hangs
    • chose a 500ms max for the autocomplete requests
  • feeds that require auth - filter them out? --interactive likely will not work in a completions context
    • handled this by having any feed communication exceptions just return empty completions for that feed, this isn't critical enough to need diagnostics/loggin g

@baronfel baronfel requested a review from a team as a code owner July 24, 2024 21:29
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Jul 24, 2024
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

1 similar comment
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@baronfel baronfel requested a review from a team July 24, 2024 21:29
@baronfel baronfel added Area-NuGet Area-CLI cli-ux Issues and PRs that deal with the UX of the CLI (exit codes, log output, verbs/options, and so on) and removed untriaged Request triage from a team member labels Jul 24, 2024
@baronfel baronfel force-pushed the add-package-version-completion branch from 7ab336e to ee09bec Compare July 24, 2024 22:21
@aortiz-msft
Copy link

@martinrrm - Would you please take a look?

@baronfel
Copy link
Member Author

@martinrrm if you want to take it for a spin yourself:

  • clone this PR
  • build with ./build.cmd or equivalent for your platform
  • run the 'dogfood' script: ./eng/dogfood.ps1 or equivalent for your platform
  • your dotnet is now the freshly-built one from this PR - make sure your shell has dotnet tab completion enabled and you should be good to go!

@anangaur
Copy link

This is great addition 👏🏽👏🏽👏🏽

@JonDouglas
Copy link

JonDouglas commented Jul 29, 2024

image

🚢 IT!!!!!!!

baronfel added 2 commits July 31, 2024 13:49
The tab completion takes into account any fragments of the version entered,
as well as the --prerelease flag.
@baronfel baronfel force-pushed the add-package-version-completion branch from a003841 to 91794e0 Compare July 31, 2024 18:55
Copy link
Contributor

@martinrrm martinrrm left a comment

Choose a reason for hiding this comment

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

Not sure if you need an approval from me, change looks great, there is just that improvement we can do, not sure if it's going to be done in this PR. Thanks for the change!

@baronfel
Copy link
Member Author

Not sure if you need an approval from me, change looks great, there is just that improvement we can do, not sure if it's going to be done in this PR. Thanks for the change!

I actually did that improvement today! So all is looking good. I'm going to ask the SDK team for code review as well then we can merge it.

@KalleOlaviNiemitalo
Copy link
Contributor

Does this deserve additions to telemetry notices? I don't remember any other tab completion in dotnet contacting network servers.

@baronfel
Copy link
Member Author

baronfel commented Aug 1, 2024

Does this deserve additions to telemetry notices? I don't remember any other tab completion in dotnet contacting network servers.

I don't think it requires any additional telemetry notices - while this is novel in the sense that it checks your configured sources for packages, you're already signalling consent to communicating with those registries during restore, etc. Are there specific kinds of data you're concerned about leaking to feed owners that we can help proactively prevent?

@KalleOlaviNiemitalo
Copy link
Contributor

No, I'm not concerned about any specific kind of data. I just feel that, even if a user would be happy to let dotnet restore or dotnet package search query a NuGet feed, doing the same during tab completion may be unexpected as the user did not run a command yet. However, setting up this tab completion requires deliberate user action, so if the completion script installation instructions mention this feature, that should be enough.

@baronfel
Copy link
Member Author

baronfel commented Aug 2, 2024

Package Id completions seem to be broken, needs more testing

@nagilson
Copy link
Member

nagilson commented Aug 2, 2024

I believe we had mentioned there was also an issue with the version completion, and that it should be tested in scenarios without a nuget.config.

@baronfel
Copy link
Member Author

We should backport this to the 9.0.1xx release branch now that it has branched off of main.

@zivkan
Copy link
Member

zivkan commented Aug 15, 2024

Was this tested with a private Azure DevOps feed?

By default, apps using NuGet.Protocol, SourceRepository will not look for credential providers. A static method needs to be called, and then SourceRepository will search for credential providers when it makes requests on feeds that need authentication. Unfortunately, I can't remember the name of the API, so I can't quickly search the code to see if it's being called. But if testing showed that private Azure Artifacts feeds don't work, let me know and I'll look it up.

@baronfel
Copy link
Member Author

It's worse than that @zivkan - the AzDo feeds don't actually support the searchautocompleteservice resource. The Registry apis will give you and AutoCompleteResource but then blow up with a protocolexception when you try to use that resource due to lack of the necessary endpoint.

I would have expected a null instance if the Source didn't actually support autocompletion.

Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

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

Turns out the previous 'issues' were with the nuget.config... this is pretty awesome! Thank you for having a clean code style and writing useful comments. I could not find any bugs. I am very excited to see this feature ship 🚀

@baronfel baronfel merged commit 4a46908 into dotnet:main Aug 15, 2024
37 checks passed
@baronfel baronfel deleted the add-package-version-completion branch August 15, 2024 23:43
@baronfel
Copy link
Member Author

/backport to release/9.0.1xx

Copy link
Contributor

Started backporting to release/9.0.1xx: https://github.com/dotnet/sdk/actions/runs/10412988686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CLI Area-NuGet cli-ux Issues and PRs that deal with the UX of the CLI (exit codes, log output, verbs/options, and so on)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants