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 config for semantic-tokens-plugin for mapping from hs token type to LSP default token type #3940

Merged
merged 27 commits into from
Jan 14, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented Jan 10, 2024

This would provide configurability for semantic tokens #3937
Things that is done:

  1. Add tokenMappings to config of semantic-token-plugin
  2. Add test to see if modification to tokenMappings is working
  3. Fix some leftover issue in add doc and ci test for semantic tokens #3938
  4. ajust this part in the global config test

default config for semantic tokens:

        "semanticTokens": {
            "config": {
                "class": "class",
                "classMethod": "method",
                "dataCon": "enumMember",
                "function": "function",
                "patternSyn": "macro",
                "recField": "property",
                "typeCon": "enum",
                "typeFamily": "interface",
                "typeSyn": "type",
                "typeVariable": "typeParameter",
                "variable": "variable"
            },
            "globalOn": false
        },

@soulomoon soulomoon marked this pull request as ready for review January 10, 2024 14:32
@soulomoon
Copy link
Collaborator Author

Configuration is implemented, @michaelpj @fendor
See if this look good ?

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

The main thing is I think it would be better with one option per mapping, although I'm not sure how to do that. Maybe @fendor knows having looked at the config stuff recently?

@michaelpj
Copy link
Collaborator

properties :: Properties '[ 'PropertyKey "mode" (TEnum Mode)]

Seems to have an example of enum properties. Looks like each one will need its own descriptions of the enum members, but you can share that on the Haskell side.

@soulomoon
Copy link
Collaborator Author

properties :: Properties '[ 'PropertyKey "mode" (TEnum Mode)]

Seems to have an example of enum properties. Looks like each one will need its own descriptions of the enum members, but you can share that on the Haskell side.

thanks for the link. Switching to this method.

@soulomoon soulomoon requested a review from 0rphee as a code owner January 12, 2024 06:27
@soulomoon
Copy link
Collaborator Author

soulomoon commented Jan 12, 2024

See if this looks good, I use Enum property for each token type now @michaelpj

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Just some comments about the naming and stuff. I think this does matter since it's user facing so it's worth making sure it's comprehensible to people

@@ -249,6 +249,622 @@
"scope": "resource",
"type": "boolean"
},
"haskell.plugin.semanticTokens.config.class": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know whether we might want more configuration options later, but perhaps it would be wise to nest all these in config.tokenMapping or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is the main remaining question. Not sure if it's going to make that much of a difference or not 🤔

I guess the alternative would be to suffix the option name with Token? So we could have config.classToken which is relatively clear what it's doing...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know whether we might want more configuration options later, but perhaps it would be wise to nest all these in config.tokenMapping or something?

I think this one would be better. I am going to extend the properties api to better support nested configuration.

{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}

module Ide.Plugin.SemanticTokens.SemanticConfig where
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this less code than defining them manually? ;) It's only 11 items. Up to you though 🤷

@michaelpj
Copy link
Collaborator

Yeah, the general output looks great

@fendor
Copy link
Collaborator

fendor commented Jan 13, 2024

Maybe @fendor knows having looked at the config stuff recently?

Sorry, I don't have any insight in the config specification itself

@michaelpj
Copy link
Collaborator

I think in the short term just suffix-ing the options is fine, e.g. classToken.

@soulomoon
Copy link
Collaborator Author

Sure, let's fix the name and try to merge this one first.
I can come back to open another pull request for the nested solution once things in #3944 is done.

@michaelpj
Copy link
Collaborator

Yes, although generally it's better to avoid changing config names since it messes up users

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Nearly there, just a couple of things that shouldn't have been checked in, then I think we can merge.

config.vscode Outdated Show resolved Hide resolved
generate.bash Outdated Show resolved Hide resolved
@soulomoon soulomoon enabled auto-merge (squash) January 14, 2024 00:01
@soulomoon soulomoon disabled auto-merge January 14, 2024 00:07
@michaelpj
Copy link
Collaborator

Has conflicts now

@michaelpj
Copy link
Collaborator

Test failures seem genuine?

@soulomoon
Copy link
Collaborator Author

🤔 fail again I'll take a look

@soulomoon
Copy link
Collaborator Author

The merge from master create some problems, need manually fixing.

@soulomoon
Copy link
Collaborator Author

soulomoon commented Jan 14, 2024

It should be fixed now

@michaelpj michaelpj merged commit 1c62ba3 into haskell:master Jan 14, 2024
36 checks passed
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