-
Notifications
You must be signed in to change notification settings - Fork 34
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
Avoid side-effects from RC_EXPAND_PARAM in async_job #15
Conversation
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 for the PR!
Would you mind adding a new test-case for this as well in async_test.zsh
, one that breaks before the patch and is fixed after? It not, don't worry, I can add it later.
async.zsh
Outdated
@@ -279,6 +279,8 @@ _async_zle_watcher() { | |||
# async_job <worker_name> <my_function> [<function_params>] | |||
# | |||
async_job() { | |||
# Reset all options to defaults inside async job. | |||
emulate -R zsh |
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.
We should change this to emulate -L zsh
and remove the setopt
below (-L
does the same, and more). Using -R
here would modify the options in the users shell since LOCAL_OPTIONS
has not yet been set.
I think simply quoting |
Good catch - it's using it in _async_worker and I must have thought it was using -L. The only reason why I would put emulate -L instead of quoting would be so in the future it's fool proof but I think with a test it's fine. I actually am not sure how performant For the test I'm fine with doing it but I don't see any tests for options at all so I'm not sure how you'd want it. How did you want to set it up? It probably should test multiple options set wherever possible with some of the more troublesome options (SHWORDSPLIT for example) but not sure if running each test with no options and then all these options is the way to go. Or did you just want a new test testing this specific option (RC_EXPAND_PARAM with async_job). |
That was my thought as well, my feeling is that fewer modified local options would be more performant, but I have nothing to back that up :).
Cool! For now I'd be fine with just a new test, e.g. Let me know if you have any questions or run into any problems with the test runner, it's home-brewed so documentation is magnificently lacking ;-). |
Added a test for it (copied mainly from the multiple options test) - it seems to work when I test it (and fails when I remove the fix) so I believe it's working if you can just confirm I did it correctly. |
Looks great, thanks! 👍 |
Landed in v1.5.1 🎉. |
It's currently causing issues with RC_EXPAND_PARAM set - essentially breaking the jobs when there are multiple arguments.
Looks like it started here: 34d6f71