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

Add support for deprecated and completion tags in complete provider #180

Merged
merged 4 commits into from
Apr 5, 2020

Conversation

lukaszsamson
Copy link
Collaborator

update elixir sense to bring EEP48 metadata support in docs

Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

All the logic looks good, I just have some minor stylistic tweaks.

items =
Enum.reject(items, fn item ->
!snippets_supported && snippet?(item)
{:snippets_supported, false} in options and snippet?(item)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{:snippets_supported, false} in options and snippet?(item)
snippets_supported = Keyword.get(options, :snippets_supported, false)
!snippets_supported && snippet?(item)

I think using functions from the keyword module will make this logic easier to understand, especially to understand the default being used.

insert_text_format(:snippet)
else
insert_text_format(:plain_text)
end
}

# deprecated as of Language Server Protocol Specification - 3.15
json =
if {:deprecated_supported, true} in options do
Copy link
Member

Choose a reason for hiding this comment

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

Please use Keyword here as well

json
end

tag_supported = options |> Keyword.fetch!(:tag_supported)
Copy link
Member

Choose a reason for hiding this comment

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

Can we change this keyword to be tags_supported? (another option would be just tag_support, but I prefer the first).

Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. Looks good! ❤️

@axelson axelson merged commit 9637725 into elixir-lsp:master Apr 5, 2020
@lukaszsamson lukaszsamson deleted the ls-docs-metadata branch April 15, 2020 08:01
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