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

Improve postfix trace #126

Merged
merged 4 commits into from
Jul 30, 2024
Merged

Improve postfix trace #126

merged 4 commits into from
Jul 30, 2024

Conversation

RblSb
Copy link
Member

@RblSb RblSb commented Jul 25, 2024

call().tr<tab>; currently generates trace(call());;, so this fixes such case with semicolon

@@ -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

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 :/

@RblSb RblSb merged commit 824a48b into vshaxe:master Jul 30, 2024
2 checks passed
@RblSb RblSb deleted the postfix-trace branch July 30, 2024 16:31
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.

3 participants