-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 #
and *
Behaviour
#702
Conversation
5ab128a
to
f576c25
Compare
const word = TextEditor.getText(new vscode.Range(start, end)); | ||
|
||
if (whitespaceRegex.test(word)) { | ||
throw VimError.fromCode(ErrorCode.E348); |
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 beginning to think we should move away from throw
and try/catch
. The more I use them the more I feel like they're just a goto statement to some arbitrary point in the codebase.
I'd much rather prefer making the return type something like string | undefined
to indicate failure and then handling that at the call site. Fortunately this change is pretty easy here.
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 prefer throw
as it's a bit more declarative of what's going on; undefined
is well, undefined and can be overloaded on what the caller should do with it. It doesn't matter in this case as the caller ignores the error, but eventually it would be nice to show this error on the status bar like how vim does it.
Did I sway you?
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 see what you're saying, but hailing from other languages with optionals, IMO returning an empty optional is as declarative as throwing an error. I like it because now the return value indicates "this function can fail" and the caller is forced to deal with it, or else get type errors.
As it is, anyone else who calls this function will have to remember that it throws or else they'll get into trouble.
I could see putting the status message on VimState actually. This seems nice to me because then we'd retain the behavior of updateView taking a VimState and rendering it to screen.
What do you think?
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.
As it is, anyone else who calls this function will have to remember that it throws or else they'll get into trouble.
Fair point. Coming from c#, the compiler would complain if you didn't properly catch an exception.
f576c25
to
0db01c5
Compare
735b5f1
to
c06dcf9
Compare
Properly fixes #569.
Behaviour of
#
and*
is expected to be: