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

windows compatibility through multisync #125

Merged
merged 3 commits into from
May 23, 2022
Merged

Conversation

bung87
Copy link
Contributor

@bung87 bung87 commented May 21, 2022

fix #121

@PMunch
Copy link
Owner

PMunch commented May 21, 2022

Maybe create a small template to avoid repeating the when: await X else: x pattern? And what in that test actually requires asynctools?

@bung87
Copy link
Contributor Author

bung87 commented May 21, 2022

Maybe create a small template to avoid repeating the when: await X else: x pattern? And what in that test actually requires asynctools?

yeah, I've thought that , I'll do that reduce code.

I make major functions multisync so it won't take stdio in main thread, can't do readFrame or sendFrame on test thread, so as nimlsp start as independent process, I think simulate actually io is fine.

nimlsp.nimble Show resolved Hide resolved
src/nimlsp.nim Outdated Show resolved Hide resolved
@@ -635,4 +661,13 @@ proc main(){.async.} =
warnLog "Got exception: ", e.msg
continue

waitFor main()
when defined(windows):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason why you've chosen to pass these in to main and therefore pass them around everywhere? You could just as easily have defined ins and outs as global variables like this as before, and then have the multisyncTask check the global variables without having to pass the extra argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multisync may complain about that, I have face some error, multisync not work properly, and it's better for unit test functions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multisync shouldn't complain, but I get the point about unit testing. I just don't like the repetition in multisyncTask outs: outs.something. But I guess without having them as global the only option would be a macro that inserted the left hand side as the first argument so you could at least write outs.multisyncTask something()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I have to take time recall my macro knowledge , now dont need extra outs pass in macro multisyncTask: outs.sendJson resp

tests/tnimlsp.nim Outdated Show resolved Hide resolved
@PMunch PMunch merged commit ad8ef18 into PMunch:master May 23, 2022
bung87 added a commit to bung87/nimlsp that referenced this pull request Aug 7, 2022
PMunch pushed a commit that referenced this pull request Aug 17, 2022
* fix #125

* gh workflow matrix add nim 1.2
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.

async stdin/stdout errors (compiling nimlsp on Windows)
2 participants