improvement: Set toggle for showInferredType to switch between true,false and minimal #1412
Conversation
| const setting = "showInferredType"; | ||
| const config = workspace.getConfiguration("metals"); | ||
| const configProperty = config.inspect<string>(setting); | ||
| const currentValue = configProperty?.workspaceValue ?? "false"; | ||
| let newValue = "true"; | ||
| switch (currentValue) { | ||
| case "true": | ||
| newValue = "minimal"; | ||
| break; | ||
| case "minimal": | ||
| newValue = "false"; | ||
| break; | ||
| case "false": | ||
| newValue = "true"; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Honestly, I'm a bit lost with this logic.
Why false turns into true and minimal into false? 😸
Do I understand it correctly:
- before launching server we do boolean to string migration in
migrateOldSetting - then we register a command and perform some additional migration into enum?
There was a problem hiding this comment.
then we register a command and perform some additional migration into enum?
That's only for toggle. Previously it was only true-> false and false -> true, but now we have 3 possible options and it's still useful to have a quick option to switch things on and off. So I decided on false -> true -> minimal -> false order.
There was a problem hiding this comment.
@tgodzik oh - got it, thanks for explanation!
There was a problem hiding this comment.
false -> true -> minimal -> false
That makes sense 👍
| const setting = "showInferredType"; | ||
| const config = workspace.getConfiguration("metals"); | ||
| const configProperty = config.inspect<string>(setting); | ||
| const currentValue = configProperty?.workspaceValue ?? "false"; | ||
| let newValue = "true"; | ||
| switch (currentValue) { | ||
| case "true": | ||
| newValue = "minimal"; | ||
| break; | ||
| case "minimal": | ||
| newValue = "false"; | ||
| break; | ||
| case "false": | ||
| newValue = "true"; | ||
| break; | ||
| } |
There was a problem hiding this comment.
false -> true -> minimal -> false
That makes sense 👍
| "false", | ||
| "true", | ||
| "minimal" |
There was a problem hiding this comment.
aren't false and true a bit unclear if value is an enum? Maybe off, minimal, all would be better? Although I guess this will be slightly incompatible change on the server side. I'm not insisting, but I'd like to hear some opinions before merging this PR.
There was a problem hiding this comment.
Sure this would make sense, but we would need to change the configuration on the Metals side first and make sure we take into account the old values. This change is more in line with the current state.
There was a problem hiding this comment.
IMO it'll be better to set up proper enum on server side with fallback to old boolean behavior. I'm not even talking about changing names "false, "true", "minimal" but to have proper ADT on server side to avoid code like userConfig().showInferredType.contains("true") bur rather write userConfig().showInferredType.contains(ShowInferredTypes.True)
…false and minimal Previously, showInferredType could only be true or false. Now it is a string value that can be also of value "minimal". Because of that I switched the setting to string enum, added an automigration part and switch toggle to go through all the options
Previously, showInferredType could only be true or false. Now it is a string value that can be also of value "minimal". Because of that I switched the setting to string enum, added an automigration part and switch toggle to go through all the options
Follow up from scalameta/metals#5284 (comment)