Fixed DocFx bug

[DigitalFlow]

Description

This PR switches the DocFx module to use Args instead of the deprecated Process functions.

• fixes Illegal Characters in Path error when using DocFx #2187

Unfortunately neither building the project using fake run nor using dotnet builds successfully.
But this was also the case for me before I did the changes.
Some hints whether the stringify functions are ok as they are would be great.

TODO

• unit or integration test exists (or short reasoning why it doesn't make sense)

  Note: Consider using the CreateProcess API which can be tested more easily, see https://github.com/fsharp/FAKE/pull/2131/files#diff-4fb4a77e110fbbe8210205dfe022389b for an example (the changes in the DotNet.Testing.NUnit module)

[matthid]

Unfortunately neither building the project using fake run nor using dotnet builds successfully.

Can you open an issue with the problem description? It is a bit unusual that this wouldn't work considering the amount of CI-Systems this stuff is running on.

[DigitalFlow]

Unfortunately neither building the project using fake run nor using dotnet builds successfully.

Can you open an issue with the problem description? It is a bit unusual that this wouldn't work considering the amount of CI-Systems this stuff is running on.

@matthid Yes, sure. For the FAKE build it seems that chocolatey was missing, we might want to add that to the build requirements.
I'll redirect the output to a file and open a case afterwards.

It would be awesome if you could check the file changes in the commit, I hope I used the Args module as it was intended.

[matthid]

@DigitalFlow My suggestion would be to go with arg-lists all the way and switch to the CreateProcess API (and add a test for this bugfix, a single one is enough).
This has the advantage that no string building is required at all.

[kblohm]

Thanks for taking care of this!
But there are some things that are not going to work properly right now:
You need something along the lines of boolParam, optionParam etc. Right now you generate arguments like overwrite as --overwrite=false or --overwrite=true instead of leaving the argument when it is false and just specifying --overwrite when it is true. Similairly for strings, you generate --ApiSourceFolder="" when nothing is specified instead of leaving the argument off. Same goes for Option.
If you need any help, let me know.

@matthid as you deprecated the params-stuff (e.g. boolParam) from the ProcessModule, is there any alternative now or should every module do its own thing? I know the ArgsModule can be used for escaping, but there is no support for actually "specifying" the arguments similair to what the ProcessModule had before.

[DigitalFlow]

@matthid: Would be interested in what to use as replacement for the stringification which was done in boolParam, stringParam, ... as well.
Adding a unit test and using CreateProcess is fine.

[matthid]

I know the ArgsModule can be used for escaping, but there is no support for actually "specifying" the arguments similair to what the ProcessModule had before.

Yes it would be nice to have the same helpers for list based parameter generation, I think @vbfox has written a library we might want to steal ;)

[vbfox]

s/steal/reference/ 😉

Joke aside you can copy paste it if you want it's MIT but I can cross license to Apache 2 if you need :) Just please steal the printf integration ( https://github.com/vbfox/FoxSharp/blob/master/src/BlackFox.CommandLine/Readme.md ) :

let cmd =
    CmdLine.empty
    |> CmdLine.append "build"
    |> CmdLine.appendIf noRestore "--no-restore"
    |> CmdLine.appendPrefixIfSome "--framework" framework
    |> CmdLine.appendPrefixf "--configuration" "%A" configuration
    |> CmdLine.appendf "-n=%s" name
    |> CmdLine.toString

Obviously I would prefer a reference as it will avoid fixing issues in two places (And I love that habit of JS ecosystem of making small dedicated libraries) but fake having some code so serialize / deserialize command lines already make it less simple than "just use it" ( vbfox/FoxSharp#1 )

[matthid]

@vbfox Yes referencing is an option but I don't think people should need to convert types between several libraries in order to start a process. I think process starting should be in the fake core.

[vbfox]

Can't you reference it in Fake.Core.Process ? is there a technical limitation or just a wish to keep fake core dependency free ?

[matthid]

@vbfox No technical limitation, more that we should have a set of types working well together and the wish to have breaking changes around them in control ;)

[matthid]

Or to be clear: I'm not against it. I just feel like having it in the repository leads to fewer issues in the long run..

[DigitalFlow]

@matthid: So how to continue? Copy the code you linked to a separate class and use it?

[matthid]

@DigitalFlow sorry we got a bit sidetracked. What this pr needs is a basic test showing what this fixes

[matthid]

any interest in finishing this?

[DigitalFlow]

Yes, definetly.
As was shown by @kblohm, some parameters are not handled properly.
So I need to either rework the logic, or use the library of @vbfox.

[matthid]

Yes just skip all manual escaping and just build a list of string is probably the easiest option

[DigitalFlow]

Yes, we did no escaping, however building the strings had some issues, for example boolean options that were passed as - -option=true instead of just passing - -option.
I have the feeling that we should have a central module for such stringifications, such as in ProcessHelper.