-
Notifications
You must be signed in to change notification settings - Fork 478
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
Initial bits for integrating the LSP API. #609
base: master
Are you sure you want to change the base?
Conversation
9e4af31
to
e6942a8
Compare
1c76e96
to
d66dd93
Compare
Looks good in general! Maybe we can actually just ignore the synchronous read problem for the first version. Also @axic had some additional ideas for unifying the callback functions. |
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.
Legacy Coding Style
All changes show entirely legacy coding style for Javascript and goes against best practices. Additionally the code is made increasingly more complicated than required with multiple nested functions which should not be const functions but well defined for typing.
- Interfaces should be moved outside of wrapper.ts file.
- no supporting tests for changes.
- no linting
Just my two cents.
a21be10
to
85a853e
Compare
@@ -79,6 +79,8 @@ function setupMethods (soljson) { | |||
const copyFromCString = soljson.UTF8ToString || soljson.Pointer_stringify; | |||
|
|||
const wrappedReadCallback = function (path: string, contents: string, error: string) { |
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.
just run npm run lint:fix
or the related package.json command to resolve these before committing.
No description provided.