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.

Sign up for GitHub

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

Jump to bottom

Improve postfix trace #126

Merged
merged 4 commits into from
Jul 30, 2024
Merged

Improve postfix trace #126

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0953f88
Improve postfix trace
RblSb Jul 25, 2024
a6822d2
Fix other cases
RblSb Jul 29, 2024
98c21cc
Fix var/final too with better eating
RblSb Jul 29, 2024
458efc7
minor
RblSb Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions src/haxeLanguageServer/features/haxe/completion/PostfixCompletion.hx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ class PostfixCompletion {
if (expr.startsWith("(") && expr.endsWith(")")) {
expr = expr.substring(1, expr.length - 1);
}
// `;` char in `foo.;` completion cases
final afterExprRange:Range = {
start: subject.range.end.translate(0, 1),
end: subject.range.end.translate(0, 2)
}
final afterExprChar = data.doc.getText(afterExprRange);

var replaceRange = data.replaceRange;
if (replaceRange == null) {
Expand Down Expand Up @@ -125,6 +131,7 @@ class PostfixCompletion {
}

if (level != Filtered) {
createTraceItem(expr, afterExprChar, add);
createNonFilteredItems(dotPath, expr, add);
}

Expand All @@ -139,6 +146,16 @@ class PostfixCompletion {
return result;
}

function createTraceItem(expr:String, afterExprChar:String, add:PostfixCompletionItem->Void):Void {
final endChar = afterExprChar == "\n" ? ";" : "";
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it be the other way around, e.g. add a semicolon if afterExprChar is not a semicolon? (not sure if looking just for \n works on Windows)
and since trace doesn't really return anything there is no need to omit that semicolon in most cases. or more precise the only case where you don't want to emit a semicolon is when one is already there.

technically we would have to read more than one character for afterExprChar but I think users can live with that - the usecase of having a stray semicolon when doing postfix completion is probably already exotic enough that we don't have to scan multiple characters past current edit-point.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my initial idea, but this works better for corner cases like [].iter(item -> item.tr<tab>) and maybe some other. I tested on Windows and it is fine in default LF mode, but it should test for "\n" or "\r", yeah.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really that useful to add ; in the first place?

Seems a bit out of scope for such snippet to me, and is usually the kind of "thought you'd like me to also do X" things that can (imo) be annoying (especially with oversight like that double ; issue 😅)

Copy link
Member

Choose a reason for hiding this comment

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

I'm also fine with no semicolon at all. the only problem with that is, that when you complete xyz.tr| to trace(xyz) your instinct is to type a semicolon. only the cursor still has xyz marked, so you will replace xyz with a semicolon, instead of adding one at the end.
it's a bit weird and I must admit I've never used trace postfix completion so maybe it takes some getting used to. the only time I use postfix completion is with switch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to mess with people's muscle memory and think that adding ; can be left if no more problems arise after this fix. After all, trace does not return values ​​and almost always requires a semicolon at the end anyway.

Copy link
Contributor

@kLabz kLabz Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that's enough

See these for example:

var foo = {bar: () -> trace("foo")};
var bar = [() -> trace("foo")]
baz(() -> trace("foo"));
baz(() -> trace("foo"), "whatever");

And variations with whitespace, including new lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of solving a lot of rare cases, I noticed the implementation of eating and improved it a bit for trace/var/final snippets. I think that's good enough for their main usage

Copy link
Contributor

Choose a reason for hiding this comment

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

How exactly does that fix any of these cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not. I decided to fix the main cases trace(0);; / final a = 0;; / var a = 0;;, the rest will require separate additions. But I don't think they are as important in the real world, as well as my example with [].iter(...).

Copy link
Contributor

Choose a reason for hiding this comment

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

The fix was not to add semicolon at all, instead of adding it that only works in simple cases :/

add({
label: "trace",
detail: 'trace(expr)$endChar',
insertText: 'trace($${1:$expr})$endChar',
insertTextFormat: Snippet
});
}

function createNonFilteredItems(dotPath:Null<DotPath>, expr:String, add:PostfixCompletionItem->Void) {
if (dotPath != Std_String) {
add({
Expand All @@ -149,12 +166,6 @@ class PostfixCompletion {
});
}

add({
label: "trace",
detail: "trace(expr);",
insertText: 'trace($${1:$expr});',
insertTextFormat: Snippet
});
// TODO: check if we're on a sys target
add({
label: "print",
Expand Down
Loading