Promise-ify nreplClient.ts. #49
Conversation
|
Thank you very much for your work @mhansen! |
|
Sorry, I'm new and I think I'm probably missing something - how do I check your review? I don't see any comments on the diffs here: https://github.com/avli/clojureVSCode/pull/49/files?diff=split. Is there a trick to show review comments? |
|
Ah, I see what you mean: I left some comments on your pull request #50. Looks good! |
src/nreplClient.ts
Outdated
| const stacktrace = (session: string): Promise<any> => send({ op: 'stacktrace', session: session }); | ||
| const stacktrace = (session: string): Promise<any> => { | ||
| return send({ op: 'stacktrace', session: session }); | ||
| } |
There was a problem hiding this comment.
Is there any reason to chance this @mhansen?
Because they mean exactly the same.
Do you think it's easier to read or something like that?
There was a problem hiding this comment.
I just changed it to be consistent with the other lines in this file, where a bunch of the other send functions have the body of the function in a block on a new line.
But I don't feel strongly about this change at all. Actually, looking back on the original state, it seems like there is some consistency that I missed in the first place - that if it's an easy one-liner, it's done as a one-liner style, but if it wouldn't fit on one line, it's done on two lines. So maybe this was unnecessary :-)
There was a problem hiding this comment.
That's ok. =) It's up to you (and @avli). I'll trust whatever you think it's best then.
If you think the old way was better, just commit those changes back. If not, that's fine as well.
There was a problem hiding this comment.
Reverted these bits.
Convert a few callers who had to wrap the callbacks in Promises to use the .then() Promise-transforming APIs instead. Removed a duplicate definition of 'document' in clojureSuggest.ts which was flagged as a warning after this refactoring.
mhansen
force-pushed
the
master
branch
2 times, most recently
from
9c864b4 to
75ac00c
Compare
|
Great job indeed, guys. Thank you very much! |
Convert a few callers who had to wrap the callbacks in Promises to use the .then() Promise-transforming APIs instead.
Removed a duplicate definition of 'document' in clojureSuggest.ts which was flagged as a warning after this refactoring.