Add additional Squirrel parameters

[pchinery]

Squirrel seems to have a few additional parameters, which I have added in this PR.

To have something to work with if things should change in future, I have also added "AdditionalArguments" to add raw arguments to the squirrel call. This is not a favorable solution to use, but rather a tool to create short-term workarounds. I'm open to discussing this part. It does help for the moment, but not having this option also has greatly accelerated me in creating the PR.

[pchinery]

With all due respect and understanding, I would like to amend my point above. I know that the FAKE team (an especially @matthid) does a terrific job at a very high pace and there are loots of completely reasons, why a PR is not merged or discussed. So I don't want to push attention to my PR when other things have a higher priority.

For these situations, it would be helpful to keep the workaround way in my PR of adding required parameters on a short, as this would have allowed me to continue with my work and would take the pressure from the team to merge it.

So again, please don't feel pushed in any way. I'm still open to discuss the "all purpose" parameter list of course, but my opinion of its usefulness has changed a bit.

[matthid]

Don't assume there are any other reasons than just missing time. Every contribution is appreciated. Sorry just give me a couple of days to review and give feedback

[pchinery]

Not a problem at all, missing time is one valid reasons for me, everyone has other work, family and time one might not want to spend on a computer. Waht I wanted to express: I fully understand any reason :)

[matthid]

Regarding the additional arguments field, afaik we have that on other modules as well. The only feedback is that is should be named similarly across modules

[matthid]

Sorry for the late review. I'd say this module needs updates to the new API and some tests. The change itself looks good to me besides the discussion about the name of the AdditionalArguments-field (the field itself is fine as we have it in lots of other modules). We could add documentation which states "if you need to use this field please consider sending a PR to FAKE in order to make life easier for everyone, thanks!"

[matthid]

It seems like we have any combination of this name already, besides the one you have chosen 💃

Expecto:

      /// Custom arguments to use in the case the helper not yet supports them
      CustomArgs : string list

DotCover:

      CustomParameters: string

OpenCover:

          /// This options is used to add additional optional arguments, could be somthing like "-returntargetcode "
          OptionalArguments : string

InnoSetup

        /// Additional parameters
        AdditionalParameters : string option

Npm/Yarn

    | Custom of string

Chocolatey

        /// A character string containing additional arguments to give to choco.
        AdditionalArgs: string

I'm not sure but at least we should be consistent and I might enforce a name via contribution guidelines. Do you like any of the existing ones?

[matthid]

Also I'm not sure if it should be a list or just a string (and leave escaping to the user).

[pchinery]

Facepalm for me... I'm sorry.

I think I would prefer AdditionalParameters. The way it is implemented right now, each parameter will be string escaped and surrounded with quotes. Not doing that is easy of course. In that case, I would change it to a string option. Do you have any preference between string option and string list? I can do the change on Sunday or Monday.

[matthid]

I was more pointing out the bad quality of the existing code base than the name you have chosen. In fact I think one more name doesn't even matter at this point, but we should work in unifying this in the next version and I'd "lock" the one we choose now in the contribution guidelines :D

There are some tools where the "escaping" will actually break them so I tend to just provide a simple string here, but I'm open for arguments for using a list, which I guess would be more "safe".

[matthid]

I'd recommend updating this to the Arguments/CreateProcess API as the StringBuilder is a bit unsafe

[pchinery]

I have now added a commit that uses the CreateProcess API and uses only a string option for the args

[matthid]

Thanks, looks good!