suggest: refactor Suggest stringify procedure

[bung87]

Summary

Move nimsuggest specific string serialization for Suggest into the
nimsuggest module itself. This is an internal refactoring, no
functional change.

Details

Creating string representations of a Suggest via $ was
nimsuggest focused,
this has been moved into the nimsuggest module and renamed
toSuggestMsg .

As part of this change, Suggest.isGlobal was removed, and global and
deprecation is now tracked via the flags field, backed by the newly
introduced
SuggestFlag enum.

[zerbina]

This is a breaking change, and I don't think is a good idea. If you run the highlight command in the following file:

var x = 0

You'll now get highlight skVar ... instead of highlight skGlobalVar .... However, @saem is more familiar with nimsuggest and its direction than I am, so I've requested a review from him.

[bung87]

like said, it's not used in anywhere, instead it will lead problem, luckily it only used for tester, this field expect stringified TSymKind but they are out of the TSymKind range.

[zerbina]

like said, it's not used in anywhere, instead it will lead problem, luckily it only used for tester, this field expect stringified TSymKind but they are out of the TSymKind range.

Something not being used anywhere is not a reason to remove something, rather it's a data point on what the impact of the removal would be. For example, there exist compiler error messages (and maybe even language features) that are never tested for, but that doesn't imply that they should be removed.

---

In any way, skGlobalVar causing problems for places that expect a valid TSymKind is a reason for why it should be changed, but you didn't mention this in the PR description.

Did you check why this feature (skGlobalVar for global variables) was introduced? The reason for why something was originally introduced usually provides additional information that can help with deciding whether something can or should be removed.

[bung87]

just found it used in vim related extension,
see
https://github.com/baabelfish/nvim-nim/blob/474e74954b79adeac0548bf8c397cf3b9b918fdc/plugin/nim.vim#L109
https://github.com/wsdjeg/vim-nim/blob/6e4f0755344b5f0ecafbe48f77819e3d307d10bf/autoload/nim/features/outline.vim#L33
they are whether commented or map to normal var and let variable. the extension may want give a different color for global variables.

[saem]

@alaviss you're the vimmer, thoughts on this?

[saem]

I think part of the issue might be the expectation that it should be a stringified TSymKind, that in and of itself seems like a mistake. Where is this expectation coming from?

[alaviss]

@alaviss you're the vimmer, thoughts on this?

My plugin allows for it to be used to differentiate in syntax coloring. Though that has to be configured by the user (or a colorscheme).

While I won't mind its removal, I don't think there's a lot of reason to do it either.

[saem]

I don't think we should make this change as it's useful information for editors.

@bung87 if you think it still makes sense to make the change, could you note down in the PR body some more context, such as: "why expect a TSymKind in that field in the first place?" I think the problem, and whether it's a real issue or a symptom of something else, is unclear at the momement.

[bung87]

updated

[saem]

• for the future, there will be flags field in Suggest, including deprecated
  as vscode currently supported, may have other flags as well, so I don't think
  we need these two values for marking value is global.

That clarified things a fair bit. For others who might not be familiar with LSP, there is a concept of DocumentSymbols (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#documentSymbol), which contain a tags field. Said field can annotate various extra bits of information and this could be used to indicate whether it's global or not.

@alaviss if it was tagged, I'm guessing that would suffice?

[alaviss]

• the suggest library is aim for supporting IDE tools like nimsuggest,
  its data structure is used in other tools, they SHOULD rely on the data
  structure not the string representation. Having these two values may
  surprise other tool's author.

The stringification proc that this PR changes is mainly used for communicating via the nimsuggest protocol. If you intend to generalize suggest for tool usage, then it might be better to move the current procedure to nimsuggest and then you can change the other one to your liking, including turning the data into a more palatable human representation.

@alaviss if it was tagged, I'm guessing that would suffice?

That'd break the protocol and if you intend to go that path, I'd recommend just drop nimsuggest and go for LSP.

Removing the special values is safe enough that it won't break any currently used code.

[saem]

This is a much better and simpler change, it's clear what's going on and it reduces cross-module dependence.

[saem]

/merge

[github-actions]

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:

---

Notes for Reviewers