-
Notifications
You must be signed in to change notification settings - Fork 29
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
Conversation
@@ -139,6 +146,16 @@ class PostfixCompletion { | |||
return result; | |||
} | |||
|
|||
function createTraceItem(expr:String, afterExprChar:String, add:PostfixCompletionItem->Void):Void { | |||
final endChar = afterExprChar == "\n" ? ";" : ""; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅)
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(...)
.
There was a problem hiding this comment.
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 :/
call().tr<tab>;
currently generatestrace(call());;
, so this fixes such case with semicolon