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

Use CiInterface/SkInterface for typeclass symbols #1592

Merged
merged 5 commits into from Mar 19, 2021
Merged

Use CiInterface/SkInterface for typeclass symbols #1592

merged 5 commits into from Mar 19, 2021

Conversation

fwcd
Copy link
Contributor

@fwcd fwcd commented Mar 18, 2021

Fixes #1581

This branch replaces the CiClass/SkClass item kinds for completions and document symbols with CiInterface/SkInterface (see the linked issue for the motivation behind this change).

Additionally, the symbol icons for type families which were previously SkClass have been replaced with SkFunction.

@Ailrun
Copy link
Member

Ailrun commented Mar 18, 2021

Just a question: does this affect anything other than icons in auto-completion popup?

@fwcd
Copy link
Contributor Author

fwcd commented Mar 18, 2021

@Ailrun Yes, the document symbol icons (e.g. in the outline and breadcrumbs of the editor) are also updated.

@Ailrun
Copy link
Member

Ailrun commented Mar 18, 2021

Oh, right. So it's all about icons, right?

@fwcd
Copy link
Contributor Author

fwcd commented Mar 18, 2021

Yup.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

It seems to me that the interface icon is more appropriate, many thanks!

@@ -69,7 +69,7 @@ documentSymbolForDecl (L (RealSrcSpan l) (TyClD _ FamDecl { tcdFam = FamilyDecl
t -> " " <> t
)
, _detail = Just $ pprText fdInfo
, _kind = SkClass
, _kind = SkInterface
Copy link
Member

Choose a reason for hiding this comment

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

This one is actually for type families, right? I think the PR description is a bit misleading...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, you're right, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to leave them as SkClass?

Copy link
Contributor Author

@fwcd fwcd Mar 18, 2021

Choose a reason for hiding this comment

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

Wasn't entirely sure about type families, but in any case we should do it consistently. Thinking about it, SkInterface seems more appropriate than SkClass for type families too, since type families are somewhat analogous to type classes, but over data instead of functions.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

🌶️ suggestion: SkFunction. Isn't a type family really a type-level function? :D

Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

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

Oops, I meant to click this one. Thank you for the PR!

@Ailrun Ailrun added the merge me Label to trigger pull request merge label Mar 19, 2021
@mergify mergify bot merged commit aad6401 into haskell:master Mar 19, 2021
@fwcd fwcd deleted the typeclass-lsp-itemkinds branch March 19, 2021 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Use CiInterface as completion item kind for typeclasses
4 participants