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

Fix/improve union(enum) completions #1719

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Fix/improve union(enum) completions #1719

merged 1 commit into from
Feb 29, 2024

Conversation

llogick
Copy link
Contributor

@llogick llogick commented Jan 17, 2024

Resolves #1707

It'd require some rework but should this be extended to anon struct inits?
Should completions include = for struct fields?

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.27%. Comparing base (7d6a9e2) to head (06e87d2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1719      +/-   ##
==========================================
+ Coverage   78.24%   78.27%   +0.03%     
==========================================
  Files          35       35              
  Lines       10512    10523      +11     
==========================================
+ Hits         8225     8237      +12     
+ Misses       2287     2286       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Techatrix
Copy link
Member

It'd require some rework but should this be extended to anon struct inits?

Could give an explanation on how completions would look like with anon struct inits?

Should completions include = for struct fields?

I would say yes otherwise it may be a bit unclear on what to type next.

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

This is missing test coverage.

src/features/completions.zig Outdated Show resolved Hide resolved
@llogick
Copy link
Contributor Author

llogick commented Jan 17, 2024

It'd require some rework but should this be extended to anon struct inits?

Could give an explanation on how completions would look like with anon struct inits?

I'm thinking it'd be identical, ieb.addExecutable(. would list the fields, just as it does with b.addExecutable(.{., but insert text would be { .field_name =.. how jarring would it be for .name to be { .field_name = ?

(If . and prev tok is = or ( offer to add the {)

Seems CodeCov didn't catch that last push, but it's there https://app.codecov.io/gh/zigtools/zls/pull/1719?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=zigtools nvm, it took it 2 edits

@llogick llogick force-pushed the qol-unionenum-compl branch from 3ad04a1 to 4241ddf Compare January 17, 2024 05:12
@xdBronch
Copy link
Contributor

im messing around with this branch a bit and noticed if i tab thru the list of completions it pastes the text including the snippet placeholders
image
this doesnt happen with any other types of snippets afaict, is this a bug? editor issue? ideally it would just fill the name of the variant when im tabbing then do the snippet on enter

@xdBronch
Copy link
Contributor

also why is this only done for tagged unions? regular unions are initialized the same way

@llogick
Copy link
Contributor Author

llogick commented Jan 17, 2024

im messing around with this branch a bit and noticed if i tab thru the list of completions it pastes the text including the snippet placeholders image this doesnt happen with any other types of snippets afaict, is this a bug? editor issue? ideally it would just fill the name of the variant when im tabbing then do the snippet on enter

VSCode just selects whichever is highlighted 🤷

@llogick llogick force-pushed the qol-unionenum-compl branch from 4241ddf to 549b718 Compare January 17, 2024 06:37
@llogick
Copy link
Contributor Author

llogick commented Feb 6, 2024

Hmm, hmm, hmm ... still undecided if the label should be name or { .name =
label

@nektro
Copy link
Contributor

nektro commented Feb 6, 2024

name imo, and then .{ . should also complete

@llogick
Copy link
Contributor Author

llogick commented Feb 7, 2024

... .{ . should also complete

it does, as long as it is after the const exe = b.addExecutable(...

label2

The amount of times I've spent back and forth debugging those situations $#^#^@^@#

The regression you made me aware is analyser.resolveOptionalChildType not working as before, eg exe.libc_file = . assigning to optional should unwrap the type.

@Techatrix
Copy link
Member

@nullptrdevs I could take care of rebasing this PR if you'd like.

@llogick llogick force-pushed the qol-unionenum-compl branch 2 times, most recently from 34e8935 to 4b52ac0 Compare February 28, 2024 19:48
@llogick
Copy link
Contributor Author

llogick commented Feb 29, 2024

@nullptrdevs I could take care of rebasing this PR if you'd like.

Yes, please do take over.

PS Dropping insertText would require some work to support incomplete fields completions.

@Techatrix
Copy link
Member

I've removed the second copy of every test that tested with and without snippets and instead used the new testCompletionTextEdit function to test completion is performing the expected text edit.
FYI insertText gets converted into textEdit before sending the response.

@llogick
Copy link
Contributor Author

llogick commented Feb 29, 2024

FYI insertText gets converted into textEdit before sending the response.

It was worth finding out, 👍

@llogick
Copy link
Contributor Author

llogick commented Feb 29, 2024

Does your approval stand on this one, merge?

@Techatrix
Copy link
Member

I have found an issue. In the following example I am doing a comparison which should only complete to alpha but completes to { .alpha = <cursor>}.

const Birdie = enum { canary };
const U = union(enum) { alpha: []const u8 };
const u: U = undefined;
const boolean = u == .<cursor>

Can be tested with:

try testCompletionTextEdit(.{
    .source =
    \\const U = union(enum) { alpha: []const u8 };
    \\const u: U = undefined;
    \\const boolean = u == .<cursor>
    ,
    .label = "alpha",
    .expected_insert_line = "const foo: U = .alphh",
    .expected_replace_line = "const foo: U = .alphh",
});

@llogick llogick force-pushed the qol-unionenum-compl branch from 87bd0b1 to 06e87d2 Compare February 29, 2024 17:27
@llogick llogick merged commit 9a3dde6 into master Feb 29, 2024
6 checks passed
@llogick llogick deleted the qol-unionenum-compl branch February 29, 2024 17:34
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.

Autocomplete union(enum) variant
4 participants