-
Notifications
You must be signed in to change notification settings - Fork 397
Add consistency to output handling in runtime code execution#583
Add consistency to output handling in runtime code execution #583rjanja wants to merge 1 commit into
Conversation
10e12b6 to
7a6ae1e
Compare
7a6ae1e to
1c7d667
Compare
rjanja
commented
Nov 1, 2018
I've been able to get the tests running locally, and the four failures reported by AppVeyor are passing for me. Not sure about those other skipped tests ("cannot run clustered test cases if distribution is not active"). I've tried kicking the build off a few times and it's the same failures every time.
bitwalker
commented
Nov 3, 2018
I think is probably reasonable - though I think my preference would probably be to always require explicit I/O for eval, but implicit for rpc. The reason is that they are generally used for different things. The reason eval requires explicit I/O is because you generally use it to build CLIs, or at least scripts to back custom comands; and in such cases you want control over the output. On the other hand, rpc is generally used to interrogate the remote node, so it is more convenient to have the result always be printed. If we really want to unify them, I would prefer to have rpc also do what eval did before, which is require explicit I/O to print the result. That said, I would like to know your thoughts on what I've just described, and whether your use cases differ, and if so, how; that may help me form a better mental model for what this should look like :)
rjanja
commented
Nov 7, 2018
The distinction makes a little more sense now that I understand the reasoning behind it. I think I'd still favor consistent I/O handling, or a command line flag to enable (or disable, if enabled was the default) the printing of return value.
One point of confusion (for me) was that eval and rpc essentially do the same thing, just in different environments (local vs remote node). The names of these tasks being so different made it harder to grok.
As for my use case, I'm adding a couple of helpers to Bootleg for invoking these release tasks with arguments. It's a WIP but the syntax is currently:
mix bootleg.eval mfa HelloWorld.ReleaseTasks.migrate/0mix bootleg.rpc mfa Application.loaded_applications/0mix bootleg.rpc expr ':application.get_key(:hello_world, :vsn)'
Would it be overkill to have as Distillery tasks:
run_localrun_remoteeval_localeval_remote
Where eval_* doesn't print but run_* does?
I'm happy to move this discussion to a new issue, and pare this PR back to just some minor documentation improvements. Let me know what you think. Thanks!
Uh oh!
There was an error while loading. Please reload this page.
Summary of changes
The node execution function
eval/2inMix.Releases.Runtime.Controlhas been modified to be consistent withrpc/2with regard to how it handles the result.In
rpc, the result is always printed to stdout. But inevalnothing is printed.Current behavior
Explicit IO
Without explicit IO
I expected "foobar" to be printed.
I suppose the inconsistency could instead be fixed by never printing the result, but that would make these commands less useful I think. If silent operation is desired, perhaps that could be added later as an optional flag.
Proposed behavior
Explicit IO
Without explicit IO
Checklist
Also:
Licensing/Copyright
I certify that I own, and have sufficient rights to contribute, all source code and
related material intended to be compiled or integrated with the source code for Distillery
(the "Contribution"). My Contribution is licensed under the MIT License.