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

fix: unify doNotSuggest #213

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

Conversation

p-spacek
Copy link

@p-spacek p-spacek commented Nov 6, 2023

What does this PR do?

related to this conversation redhat-developer/yaml-language-server#911 (comment)

exclude doNotSuggest schema from autocompletion
doNotSuggest was already there but implemented only for the simple object properties.
This PR uses doNotSuggest also for other scenarios: anyOf, values

"anyOf":[
  {
     "$ref": "schema1",
     "doNotSuggest": true
   },
  {
     "$ref": "schema2",
   }
]

PROBLEM

this PR contains also a fix for the test utility testCompletionsFor, there is an issue with checking zero suggested items.
the previous implementation doesn't allow to check 'no result' with expected: { count: 0}
but when I fixed the test utils, the test In comment fails

Options:

  • use this PR after the fix of the comments
  • if you give me advice on how to check empty results, I can modify this PR (but the comment issue remains)

Is it tested? How?

adds unit tests

@@ -74,7 +74,7 @@ suite('JSON Completion', () => {
const position = Position.create(0, offset);
const jsonDoc = ls.parseJSONDocument(document);
return ls.doComplete(document, position, jsonDoc).then(list => {
if (expected.count) {
if (typeof expected.count === 'number') {
Copy link
Author

Choose a reason for hiding this comment

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

if you call this method with expected.count === 0 it won't check zero length (assert.equal(list!.items.length, expected.count)

but after this change, it seems that test for test('In comment', ... is not correct

@p-spacek
Copy link
Author

p-spacek commented Nov 7, 2023

@microsoft-github-policy-service agree

@aeschli
Copy link
Contributor

aeschli commented Nov 27, 2023

@p-spacek There's a test failure, if you can have a look...

@p-spacek
Copy link
Author

@aeschli check the description and code comment please,

Yes, there is an issue with comment implementation. The issue was there before.
The reason why it fails now is because I fixed the utility for testing.
The reason why it didn't fail before is that the utility test incorrectly compared an empty array.

So to be honest, I am not sure how to fix it
I can modify this PR and remove the 'test utility fix' - to hide the comment issue and change my test not to use this test utility
Or I can update the failing comment test to expect two results - instead of 0
I don't dare to fix the comment issue - proper fix.

I am sorry, I shouldn't combine 2 different things together (feature + issue(failing test)), it's confusing now...

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.

2 participants