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

Process parameter in e.g. CommandService::run is difficult to comprehend #343

Open
frauzufall opened this Issue Nov 4, 2018 · 2 comments

Comments

Projects
None yet
3 participants
@frauzufall
Copy link
Member

frauzufall commented Nov 4, 2018

I want to raise a general issue with the run methods of the CommandService and the Moduleservice (and maybe other places too?).

Future<CommandModule> CommandService::run(String className, boolean process, Object... inputs);

The boolean process parameter for enabling or disabling post- and preprocessing is a bit lost there. It took me a long time to fully understand what pre- and postprocessing really means and I have talked to quite a few developers in this ecosystem who still don't know. If you read code containing some exemplary run call, it is not self-explanatory what this boolean in between the command name and arguments is representing.

Could we not split these two cases into two functions (naming open for discussion)?

Future<CommandModule> CommandService::run(String className, Object... inputs);
Future<CommandModule> CommandService::runNoProcessing(String className, Object... inputs);

They would not interfere with the old version which could be deprecated but still provide backwards compatibility, right?

@frauzufall frauzufall changed the title `Process` parameter in `run` calls is difficult to comprehend Process parameter in e.g. CommandService::run is difficult to comprehend Nov 4, 2018

@haesleinhuepf

This comment has been minimized.

Copy link
Member

haesleinhuepf commented Nov 4, 2018

Hey @frauzufall,

great suggestion! I think ScriptService could profit from the same API improvement 🙂

Cheers,
Robert

@imagejan

This comment has been minimized.

Copy link
Member

imagejan commented Nov 5, 2018

Just for some context on pre- and post-processing, please also see the open issues #191 and #317.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment