r1pper / Topshelf.StartParameters

A small extension that enables Topshelf based services to have start parameters
MIT License
28 stars 7 forks source link

fix: Removing unnecessary prefix from WithStartParameter #7

Open vesuvian opened 5 years ago

vesuvian commented 5 years ago

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.

r1pper commented 5 years ago

@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.