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

feat: exclude deprecated schema from autocompletion #833

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

p-spacek
Copy link
Contributor

What does this PR do?

exclude deprecated schema from autocompletion
useful when you want to exclude some subschemas from autocompletion, but you need it to be there for validation purposes. So you can't just remove it.

"anyOf":[
  {
     "$ref": "schema1",
     "deprecatedMessage": "Deprecated: use schema2 instead"
   },
  {
     "$ref": "schema2",
   }
]

What issues does this PR fix or reference?

no ref

Is it tested? How?

added tests

@coveralls
Copy link

coveralls commented Jan 17, 2023

Coverage Status

Coverage: 83.373% (+0.07%) from 83.304% when pulling 6dc3ca9 on jigx-com:feat/exclude-deprecated-schema-from-autocompletion into cf3f792 on redhat-developer:main.

@gorkem
Copy link
Collaborator

gorkem commented Jan 24, 2023

This should be setting to exclude them. Otherwise, LSPs do have a way to mark deprecated code completions as deprecated and we should actually use that mechanism instead of excluding them directly.

@p-spacek
Copy link
Contributor Author

p-spacek commented Feb 7, 2023

This should be setting to exclude them. Otherwise, LSPs do have a way to mark deprecated code completions as deprecated and we should actually use that mechanism instead of excluding them directly.

Hi @gorkem , Sorry, I am not sure if I understand correctly your thoughts. Can you explain them, please?
The previous implementation allows only to exclude properties, not the whole schema.

@p-spacek p-spacek force-pushed the feat/exclude-deprecated-schema-from-autocompletion branch from f4335f9 to ff6a896 Compare February 7, 2023 09:09
@gorkem
Copy link
Collaborator

gorkem commented Feb 25, 2023

Deprecated APIs are common in programming languages. Typically language tooling deals with it, not by removing them without notice but by marking them. Deprecated properties can be marked as deprecated by the yaml-language-server using the deprecated hint. #763 is an incomplete implementation in that direction. We can also take it further and introduce a setting that will allow users to select that they do not want deprecated properties to be presented as code completion.

@p-spacek
Copy link
Contributor Author

Oh, I understand now. Sorry for the late reply.
So I think that I need doNotSuggest schema property to exclude something from code-completion

Note1: current implementation excludes properties with deprecationMessage
https://github.com/redhat-developer/yaml-language-server/blob/main/src/languageservice/services/yamlCompletion.ts#L719
if (typeof propertySchema === 'object' && !propertySchema.deprecationMessage && !propertySchema['doNotSuggest']) {

So I will probably create another fix PR that will finish the doNotSuggest implementation

  • doNotSuggest is implemented only for properties, but it should work also for schemas (value and property completion)

do you agree, @gorkem ?

I won't modify the deprecation functionality at all for now...

  • maybe later as you suggested:
    • cross it out (typescript has the same)
    • some user configuration to define if a completion item should be excluded completely or crossed out

@p-spacek p-spacek force-pushed the feat/exclude-deprecated-schema-from-autocompletion branch from ff6a896 to d9b23dd Compare April 26, 2023 11:46
@gorkem
Copy link
Collaborator

gorkem commented Apr 27, 2023

A complete solution would be to enable the deprecated hint (the cross out) and add a setting (not a schema property) to not suggest the deprecated properties. I do not think it is the schema author's decision whether to use a deprecated property or not. Schema author already has done its responsibility by marking it deprecated.

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