Skip to content
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

WIP: new DocFxModule for Fake5 #1872

Merged
merged 2 commits into from
Apr 22, 2018
Merged

Conversation

kblohm
Copy link
Contributor

@kblohm kblohm commented Apr 15, 2018

Hi,
i rewrote the old DocFxHelper and added a lot of missing API.
I do however have a few remaining questions:

  • I added a new Module for Fake5 to Fake.Documentation.DocFx. I am not sure if that is the desired naming since FSFormatting is under Fake.DotNet.FSFormatting. There are however a few more Documentation modules and i dont think putting all of them under DotNet is the best idea.
  • In visual studios folder structure, should new modules be added to src/app or just the app folder?
  • DocFx has some parameters (for example for pdf generation) that reuse other parmeters. In the docfx repo they model this using inheritance but that is not too nice in fsharp especiall when using setParams functions to update the default parameters. Right now i include the additional parameters inside the "parent" parameter record. E.g. the pdfParams has a property buildParams. I could also use two setParams functions instead. That would have the advantage of not having to update nested records. I am not sure which approach u prefer here, so please let me know :)

I also have a few general questions:

  • The Tools.findToolInSubPath does not search in the new .fake\build.fsx paket packages folder. At least not as one of the first search paths. Is that the desired behavior?
  • I could not manage to make the new "inline" paket notation install the docfx.console nuget package in a location i could find with Tools.findToolInSubPath. Do u know what i am doing wrong here?

Thanks!

@matthid
Copy link
Member

matthid commented Apr 15, 2018

Thanks a lot for your PR!
Let me try to answer/discuss your points:

I added a new Module for Fake5 to Fake.Documentation.DocFx. I am not sure if that is the desired naming since FSFormatting is under Fake.DotNet.FSFormatting. There are however a few more Documentation modules and i dont think putting all of them under DotNet is the best idea.

I don't know DocFx, is that not .NET only? Can it generate stuff for other eco-systems?

In visual studios folder structure, should new modules be added to src/app or just the app folder?

Actually that is something we probably should clean-up. We have those two structures because the Fake.sln file moved from src/ to the root directory... As I'm usually just doing dotnet sln Fake.sln add src/app/NewModule.fsproj it is probably better to move everything to src/app. Feel free to do so ;) Otherwise just put it wherever you feel like ;)

I am not sure which approach u prefer here, so please let me know :)

We have this problem on various other places and I don't think we have a uniform solution to that. For example look at the Fake.DotNet.Cli module which shares some common stuff over various commands and uses nested Records (and probably SRTP functions) for that...

Another example is that we often have the need to set environment variables or Working directory. The Fake.Core.Process module provides some SRTP helpers for that:

https:/fsharp/FAKE/blob/800579e69ce35b77345ee21923ddf117168067cd/src/app/Fake.Core.Process/Process.fs#L367-L384

All you need to provide to use them is the WithEnvironment static member:

https:/fsharp/FAKE/blob/800579e69ce35b77345ee21923ddf117168067cd/src/app/Fake.Core.Process/Process.fs#L125-L126

But yeah sadly there is no "perfect" solution for that I'm completely happy with. Generally we try to make it "easy" on the call side if possible ...

The Tools.findToolInSubPath does not search in the new .fake\build.fsx paket packages folder. At least not as one of the first search paths. Is that the desired behavior?

Generally I do not expect people to use that path. I'd assume most people follow the storage:none guidance and then stuff is somewhere in the users directory. Maybe it makes sense for FAKE to provide NUGET_PACKAGES environment variable and some "parsed" lockfile object in order to find the tools we are looking for?

I could not manage to make the new "inline" paket notation install the docfx.console nuget package in a location i could find with Tools.findToolInSubPath. Do u know what i am doing wrong here?

Yes in "inline-paket" I changed the semantics a bit such that storage:none is the default (if none is given) this is in contrast to a dependency-file where storage:packages is the default. The documentation is not optimal on this, but as stated above I don't really expect people to change back to this behavior.
Regarding the path-resolution see my suggestion above. (Generally in new dotnet-core world I expect more and more tools to be powered by the dotnet-cli tooling, which can than be invoked by fake)

Just to note: The other easy way is to just not use "paket-inline" dependencies but to reference a paket group. In this scenario you have the packages folder right where it used to be in paket.

Fake.sln Outdated
@@ -4,109 +4,111 @@ VisualStudioVersion = 15.0.27130.2026
MinimumVisualStudioVersion = 15.0.26124.0
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "app", "app", "{7BFFAE76-DEE9-417A-A79B-6A6644C4553A}"
EndProject
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Fake.Core.Context", "src/app/Fake.Core.Context/Fake.Core.Context.fsproj", "{D3D92ED7-C2B9-46D5-B611-A2CF0C30C8DB}"
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Fake.Core.Context", "src\app\Fake.Core.Context\Fake.Core.Context.fsproj", "{D3D92ED7-C2B9-46D5-B611-A2CF0C30C8DB}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just replace \ with / in this file and travis will probably start to work again ;)

@kblohm
Copy link
Contributor Author

kblohm commented Apr 20, 2018

Hi,
sorry for the late reply.

don't know DocFx, is that not .NET only? Can it generate stuff for other eco-systems?

I am pretty sure the main target is .NET. The tool is used for a lot of the Microsoft-Docs, for example Asp.Net Core. The newest version should even work with F#. It does also work for Javascript and Typescript.

We have this problem on various other places and I don't think we have a uniform solution to that.

Not that that is a big problem, but it might be nice to have a common guideline. I looked at Cake and that does seem to be pretty uniform on how to do stuff. The resolution of a Tools path for example. It does seem like Fake uses a lot of different approaches there. I kinda like the update function to set settings but it does lack when u have nested structures. Maybe mutable parameter objects/records might be easier to use here. At least until there is proper language support for nested records.

Generally I do not expect people to use that path. I'd assume most people follow the storage:none guidance and then stuff is somewhere in the users directory. Maybe it makes sense for FAKE to provide NUGET_PACKAGES environment variable and some "parsed" lockfile object in order to find the tools we are looking for?

I am currently using the paket approach. But i think it would be nice to be able to use standalone Fake with a single file. It is not super important though.

Yes in "inline-paket" I changed the semantics a bit such that storage:none is the default (if none is given) this is in contrast to a dependency-file where storage:packages is the default. The documentation is not optimal on this, but as stated above I don't really expect people to change back to this behavior.
Regarding the path-resolution see my suggestion above. (Generally in new dotnet-core world I expect more and more tools to be powered by the dotnet-cli tooling, which can than be invoked by fake)

So i can just add a storage:packages and use a single file? That would be nice. The cli tooling would also be great. But that might take a while for a lot of packages.

Just replace \ with / in this file and travis will probably start to work again ;)

Can i somehow tell VisualStudio to use proper forward-slashes? It did all that automatically, i did not even realise it :)

I will fix that later / tomorrow. I will also do some more cleanup. I am however still not really sure how to handle the nested options ;). I can just keep it as is, but that is also not very pretty.

@kblohm
Copy link
Contributor Author

kblohm commented Apr 21, 2018

Ok i changed some stuff. But apparently i am too stupid to do a proper rebase :/. I do not think the yarnhelper commit should be in there.

@kblohm
Copy link
Contributor Author

kblohm commented Apr 21, 2018

Ok, i am only making this worse!
Would be nice if you could help me with this :p. Sorry!
Edit: Ok Maybe i got it this time :)

@matthid
Copy link
Member

matthid commented Apr 22, 2018

The newest version should even work with F#. It does also work for Javascript and Typescript.

Ok lets go with Fake.Documentation.

But i think it would be nice to be able to use standalone Fake with a single file. It is not super important though.

Yes I agree, and yes this is something we probably can add after release.

So i can just add a storage:packages and use a single file?

Yes, however I consider stuff within the .fake directory as implementation detail so it might change ;)

Can i somehow tell VisualStudio to use proper forward-slashes?

At least I don't know how. Ideally backward slashes would properly work everywhere (as that is just the format)

Would be nice if you could help me with this

Everything looks good now?

@matthid matthid changed the base branch from master to rc_6 April 22, 2018 08:56
@kblohm
Copy link
Contributor Author

kblohm commented Apr 22, 2018

Yes, thanks for your help.

@matthid
Copy link
Member

matthid commented Apr 22, 2018

Thanks!

@matthid matthid merged commit 775a33c into fsprojects:rc_6 Apr 22, 2018
@kblohm kblohm deleted the newDocFxModule branch April 22, 2018 09:26
@matthid matthid mentioned this pull request Apr 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants