Skip to content
This repository has been archived by the owner on Oct 9, 2024. It is now read-only.

Update module completion return statement to support FQCN #57

Merged
merged 6 commits into from
Oct 13, 2021

Conversation

priyamsahoo
Copy link
Contributor

  • The PR has the logic to support FQCN in module auto-completion. Whenever useFullyQualifiedCollectionName is set to true (which by-default is), the user will get auto-completions for module names based on their FQCN.
    For e.g., writing ansible.builtin would give suggestions for all the modules that are under the ansible.builtin collection namespace.

  • The auto-completion is smart enough to understand shortcuts while writing module names and provides the correct auto-completion accordingly.
    For e.g., writing ansiping (or something similar) would give you ansible.builtin.ping as the first suggestion. Similarly, writing iosping (or similar) woould give you cisco.ios.ping as the first suggestion.

  • Resolves [ansible-language-server] Add task completion name to work with collection completions.  vscode-ansible#145

@priyamsahoo priyamsahoo marked this pull request as ready for review October 8, 2021 05:50
@priyamsahoo priyamsahoo requested review from tomaciazek and a team as code owners October 8, 2021 05:50
Copy link
Contributor

@tomaciazek tomaciazek left a comment

Choose a reason for hiding this comment

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

I actually really like the way it currently behaves, so that even if the FQCNs are best practice and should be placed in code, the user can just focus on writing the name of the actual component that he uses. And since that is a matter of personal preference, I'd suggest implementing a feature toggle (setting) for that.

@cidrblock
Copy link

@tomaciazek We do want to encourage the use of full names, and the toggle is still there. I'm not sure I understand your remark here...

Are you suggesting an additional toggle after this goes in?

@ssbarnea
Copy link
Member

ssbarnea commented Oct 9, 2021

If logic autocomplete no longer works when trying to type builtin modules without prefix I would be against this change.

While I am totally ok to always put the FQCN inside the files, I do also think that when using auto-complete our language server should be smart-enough to make the builtin implicit and add it when user presses enter. Example, typing "deb" should autocomplete with ansible.builtin.debug.

I still need to compile it locally and test it myself.

@priyamsahoo
Copy link
Contributor Author

If logic autocomplete no longer works when trying to type builtin modules without prefix I would be against this change.

While I am totally ok to always put the FQCN inside the files, I do also think that when using auto-complete our language server should be smart-enough to make the builtin implicit and add it when user presses enter. Example, typing "deb" should autocomplete with ansible.builtin.debug.

I still need to compile it locally and test it myself.

It works. I can show you the screenshot of it.
Screenshot from 2021-10-11 09-43-05 (1)

@ssbarnea ssbarnea added the enhancement New feature or request label Oct 11, 2021
@tomaciazek
Copy link
Contributor

@tomaciazek We do want to encourage the use of full names, and the toggle is still there. I'm not sure I understand your remark here...

Are you suggesting an additional toggle after this goes in?

I'm not against full names, that default is already changed and the user will get FQCN with autocomplete. This PR, however, changes the sorting and filtering, so that collection namespaces and collection names will be matched and displayed first.

@priyamsahoo, your example is misleading - it only shows built-in debug first because it is a longer match. If there was a namespace or collection starting with debu, that would have been shown first.

The new toggle I propose would be to influence this sorting/filtering behavior.

@priyamsahoo
Copy link
Contributor Author

I have updated the priority and kept the name in the first priority even when the useFQCN is set to true.
The only change that happens with this PR now, is, the user will additionally be able to filter the suggestions based on FCQNs as well.
In general scenario, filtration of suggestions should work as usual (as in the case when useFQCN is set to false).

@priyamsahoo
Copy link
Contributor Author

Also, I am linking a new issue (a feature to add on) that I created to support config options to set priorities (for filtration of suggestions in module FQCN auto-completions) if the user wishes to.

Issue: ansible/vscode-ansible#1146

@ssbarnea
Copy link
Member

@tomaciazek is the new version ok now? If so, please remove the requested changes so we can merge it.

? `${priority}_${moduleFqcn}`
: `${priority}_${name}`,
filterText: useFqcn
? `${name} ${moduleFqcn} ${collection} ${namespace}` // name should have highest priority (in case of FQCN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need that ternary here at all? collection and namespace are anyway part of moduleFqcn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ternary is needed because when the user opts for suggestions based on FQCN, he/she should be able to prioritize the filtration (which is something we will be working post this: #62).

Also, filtration based on moduleFQCN whole together and filtration based on collection and namespace separately would have different behaviors and user should have control over it (since we decided to give that option).

Copy link
Contributor

Choose a reason for hiding this comment

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

If the user types in debug, ${name} will be matched. If they type ansible.builtin.debug, ${moduleFqcn} will be matched. I don't see a useful scenario in which ${collection} ${namespace} would be matched... unless it is an attempt to force a longer partial match (with duplicates) to elevate the entry on the sorted list. Is it?

src/providers/completionProvider.ts Show resolved Hide resolved
@tomaciazek
Copy link
Contributor

The only change that happens with this PR now, is, the user will additionally be able to filter the suggestions based on FCQNs as well. In general scenario, filtration of suggestions should work as usual (as in the case when useFQCN is set to false).

  1. That was always possible since moduleFqcn was part of the filterText.
  2. Obviously, it is not the only thing changed here.

@priyamsahoo
Copy link
Contributor Author

The only change that happens with this PR now, is, the user will additionally be able to filter the suggestions based on FCQNs as well. In general scenario, filtration of suggestions should work as usual (as in the case when useFQCN is set to false).

  1. That was always possible since moduleFqcn was part of the filterText.
  2. Obviously, it is not the only thing changed here.

I agree it was there. I was a little wrong with my words. Let me re-iterate: "With the current changes, the user now would be able to see the FQCN filtration in a more pronominal way, which in tern, would help the user to provide right set of characters, for a right suggestion"

Other than this, the PR contains additional changes to enhance the user experience for auto-completions based on module FQCN.

Before:

Screenshot from 2021-10-12 23-20-59

With current change:

Screenshot from 2021-10-12 23-21-43

@ganeshrn ganeshrn merged commit 18c0b2a into ansible:main Oct 13, 2021
@ganeshrn
Copy link
Member

I actually really like the way it currently behaves, so that even if the FQCNs are best practice and should be placed in code, the user can just focus on writing the name of the actual component that he uses. And since that is a matter of personal preference, I'd suggest implementing a feature toggle (setting) for that.

Raised an issue to track configuring sort order ansible/vscode-ansible#1150

@priyamsahoo priyamsahoo deleted the collection-completion branch September 13, 2022 13:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ansible-language-server] Add task completion name to work with collection completions.
5 participants