-
Notifications
You must be signed in to change notification settings - Fork 68
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: make reading of child message packets more robust #170
fix: make reading of child message packets more robust #170
Conversation
fe97dd4
to
7e6f63f
Compare
7e6f63f
to
0602ba4
Compare
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.
Thanks!
Just a quick note from me, since I won't be able to look at this soon.
Eask
Outdated
(script "test" "echo \"Error: no test specified\" && exit 1") | ||
|
||
(source "gnu") | ||
(source "melpa") |
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.
GNU ELPA packages are expected to avoid dependencies on MELPA.
What is this needed for? If it's buttercup
, then it is also distributed via NonGNU ELPA: https://elpa.nongnu.org/nongnu/buttercup.html
Although I always forget whether it's okay for GNU ELPA to depend on NonGNU ELPA (maybe someone else remembers?).
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 doesn't depend on it, only for tests. Those can be safely ignored, since they are not packaged, no?
I see it as a dependency for development only. I switched it to nongnu for now.
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 doesn't depend on it, only for tests. Those can be safely ignored, since they are not packaged, no?
FWIW async-test.el
is currently packaged.
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.
Yea, but it's not really a test 😅 More like a cookbook.
0602ba4
to
9783eb4
Compare
Merged as well, thanks! |
Ugh, I see that two of the windws tests failed, eventhough the same code passed in my repo... it's still a bit flaky there, maybe just the timeouts need to be bigger for windows since the process handling is a tad bit slower there. I'll try to look into it. |
This patch introduces more robust message parsing as discussed in #168 . Also adds an automatic test suite and switches
start-process
tomake-process
to separate stdout and stderr (as this mixes up on non-linux platforms in curious ways).Fix #169
Fix #131
Fix #104
Fix #142
Example of the pipeline https://github.com/Fuco1/emacs-async/actions/runs/4404697124