zigtools / zls

A Zig language server supporting Zig developers with features like autocomplete and goto definition
MIT License
2.79k stars 281 forks source link

Should completions be sorted by name? #1792

Open Techatrix opened 6 months ago

Techatrix commented 6 months ago

Completions are sorted by the label since 1805837067cc68d563866cbf1eb5873dd64aa93a. Grouping them based on what kind (keyword, function, variable, ...) they are seems reasonable but sorted them alphabetically? You often group/order declarations in a way to make them easier to understand which gets undone by the sorting.

relevant code: https://github.com/zigtools/zls/blob/8cca7a1fa125494cd840569160c6373d41fce0b6/src/features/completions.zig#L957-L966

nektro commented 6 months ago

imo they should be by 1) Leven distance from the test text or 2) source order of fields. on a personal note, ngl I got surprised by this because I thought editors took care of the ordering for you

SuperAuguste commented 6 months ago

I agree that Levenshtein distance and order of fields could both be interesting metrics to try sorting by.

On editor handling ordering:

Editors do handle sorting for you as the spec says:

[...] the client is responsible for filtering and sorting. [...] However servers can enforce different behavior by setting a filterText / sortText

We override the sort text so we order certain types of completions above others. This gives us more control but means we have to implement more ourselves vs the editor implementing it for us.

btipling commented 5 months ago

image I would expect entity here to appear before an import statement for a file I'm not currently using. I'm not sure if this is what is being discussed in this issue, but seemed similar, an ordering issue on a completion. I think it would save me a lot of time in aggregate, as I use entity a lot in my ECS based game and it always appears beneath the import. I'm also assuming this thing in my screenshot is ZLS related, sorry if it's not. I assumed it is because it's zig. I don't really know how vs code extensions work.

Techatrix commented 5 months ago

I would expect entity here to appear before an import statement for a file I'm not currently using.

If the import statement is unused then why not remove it so it doesn't appear in the completions?

I do agree that entity should come first but for a different reason. I assume that in your code entity which is of type u64 can coerce to the second parameter type of has_id which is entity_t. This means that calling ecs.has_id(...,entity,...) is valid while ecs.has_id(...,entities,...) isn't so it becomes clear which one should be prioritized.

This is a far more complex (but better) mechanism than what this issue is about so created a separate issue in #1845.

btipling commented 5 months ago

Yes entities is a file struct. and the argument to flecs entity_t is coerced from u64 in zig. You are correct that I should remove unused imports, but I often forget to, but I guess them appearing in completion should be a reminder! I have removed it. Thanks for creating the other issue I will follow that one too.

alichraghi commented 1 month ago

a bit irrelevant but i think declarations should appear before keywords. the main reason is because currently allowzero is the first result when typing allo where you're typing for allocator. most keywords are also very short and barely need completion