Topshelf.StartParameters icon indicating copy to clipboard operation
Topshelf.StartParameters copied to clipboard

fix: Removing unnecessary prefix from WithStartParameter

Open vesuvian opened this issue 6 years ago • 1 comments

Addressing the issue raised here: https://github.com/r1pper/Topshelf.StartParameters/issues/5

Looks like only one of the extension methods were adding the prefix, seems to make sense to remove the prefix for clarity and consistency. I was certainly surprised to find the prefix added to all of my parameters.

vesuvian avatar Apr 10 '19 13:04 vesuvian

@vesuvian Hi and sorry for the very late answer and thanks for the both PRs

First of all, I completely agree that it is confusing, therefore I changed the argument names to improve the situation.

I also added the public static HostConfigurator WithStartParameter(this HostConfigurator configurator, string installName, string runtimeName,Func<string, string> installAction, Action<string> runtimeAction) method which does the same thing as the Prefix one but in an explicit way, now back to the problem.

There are two main Reasons for this behavior

  1. Topshelf services can be used as simple executables which means we can use them without installing them
  2. AddCommandLineDefinition method accepts Action callback which enables the developers to write specific behaviors for their arguments.

The Prefix job is to separate the Install and Run scenario, Let's imagine we implement the WithStartParameter like (very similar to the PR):

        public static HostConfigurator WithStartParameter(this HostConfigurator configurator, string name,
            Action<string> action)
        {
            
            configurator.AddCommandLineDefinition(name,s=> 
            {
                Add(configurator, Prefix + name, s);
                action(s);
            });

            return configurator;
        }

and an Action callback as:

x.WithStartParameter("auto", n => InitializeModules(n));

in this case, the callback should only get called in the Run scenario but with the code above it is getting executed in both Run and Install.

but If you guys find it annoying, we can remove it completely and only use the new method which explicitly declares the behaviors.

r1pper avatar Jul 14 '19 15:07 r1pper