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

Upgrade FSharp.Formatting / fsdocs #369

Closed
wants to merge 1 commit into from
Closed

Conversation

adz
Copy link
Contributor

@adz adz commented Oct 10, 2020

This changes the doc structure to suit fsdocs, so we can use the command line version, and get improved layout as per https://fsprojects.github.io/FSharp.Formatting/upgrade.html

With this change, you can build a local web-served version of docs (once built, it opens the browser to the local docs, and I get local paths, and instant rebuild as I save - the docs, but not the api ref docs unfortunately):

dotnet fsdocs watch --projects src/FSharpPlus/FSharpPlus.fsproj --input docsrc/

Using this variant builds to the docs folder:

dotnet fsdocs build --projects src/FSharpPlus/FSharpPlus.fsproj --input docsrc/ --output docs/

@wallymathieu at first I thought we didn't need the code in docsrc/tools any more since the docs can be generated via this command, but then I noticed the plantuml replacement, and there is the github-pages releasedocs target. I can follow the code that does the work, but I am not sure what to do to integrate it.

I'm thinking we:

I'm not sure you agree, or have a better idea, as I"m also unsure of how the nuget release is done, etc.

<li class="nav-item" id="fsdocs-repository-link"><a class="nav-link" href="{{fsdocs-repository-link}}">Source Repository</a></li>

<!-- Do not use 'fsdocs-list-of-documents' which currently cannot be ordered, and dumps all files in docsrc/ -->
<li class="nav-header" style="padding-top: 10px">Documentation</li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default template has {{fsdocs-list-of-documents}} but this doesn't work well, since it just lists all the files in docsrc/* and dumps them into the menu (so all the type-* files, and abstraction-*). A lot of them don't produce titles, and you can't order them.

I decided to put these manually there and the result is pretty good:
image

Copy link
Member

Choose a reason for hiding this comment

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

This looks nice 👍

@@ -9,102 +9,176 @@ module Operators =

// Common combinators

/// Creates a new function with first two arguments flipped.
/// <summary>Creates a new function with first two arguments flipped.</summary>
/// <category>common</category>
let inline flip f (x: 'T) (y: 'V) : 'Result = f y x
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These categories are only partially done, but give a nice table of contents. It's no big issue since functions without a category get put into "Other module members":
image

@adz adz requested a review from wallymathieu October 10, 2020 04:37
@wallymathieu
Copy link
Member

The docs are built independently of nuget releases.

@wallymathieu
Copy link
Member

I'm trying to run the tool but I get:

cracking projects...
  skipping project 'FSharpPlus.fsproj' because it doesn't have a target path

Perhaps related to this issue:
fsprojects/FSharp.Formatting#574

@wallymathieu
Copy link
Member

The main reason from the doc generation is right now:

  • Documentation written in fsx files
  • Visual overview of the abstractions as seen in the plantuml file
  • API docs

@wallymathieu
Copy link
Member

I might be able to help out to see about getting some of the things to build. Perhaps we could see about how we should render the plantuml, it might be related to a similar question around using charts in fsprojects/FSharp.Formatting#611

@adz
Copy link
Contributor Author

adz commented Oct 10, 2020

Oh sorry @wallymathieu

Add this to the command, or set it in env somehow:

 TargetPath=src/FSharpPlus/bin/Debug/netstandard2.0/FSharpPlus.dll 

I had to do this, as was getting the same error, but after a full build of solution, I didn't seem to need it anymore.

In regards to your comment:

The main reason from the doc generation is right now:

Documentation written in fsx files
Visual overview of the abstractions as seen in the plantuml file
API docs

This fsdocs command does the 1st and 3rd, but not the 2nd.

We could keep the doc project, and have it call out to fsdocs, then follow up with plantuml, but I was thinking it might be better to use a top level build.fsx fake script.

@wallymathieu
Copy link
Member

We used to have a FAKE script to generate the docs. I think there was some issues that made it easier to avoid FAKE and paket.

@adz
Copy link
Contributor Author

adz commented Oct 10, 2020

Yes it was a bit messy pre dotnet core. As well, the build file was more complicated than needed. Now with v3 local tools it seems OK.

Also the doc project we have now seems to pull in some fake code anyway.

If docs are independent I'll have a try when I can in the next few days.

@wallymathieu
Copy link
Member

I'm unsure if that was the issue. Let's see if we can get this to work without taking a dependency on paket for everything (since that impacts the full build). The recent iterations of f# formatting have some nice improvements that we can benefit from 🙂

@wallymathieu
Copy link
Member

wallymathieu commented Oct 10, 2020

The doc project we have now have a local dumbed down version of FAKE defined in the tools folder. I've pushed changes to appveyor and travis configuration in order for them to build the docs for this branch.

So what's left right now in this PR?

  • How to deploy to docs (I'm guessing that there are some examples of this in other projects)
  • Visual overview of the abstractions as seen in the plantuml file (I'm guessing that this is the most painful thing)
  • Make sure that the build is green

@gusty
Copy link
Member

gusty commented Oct 10, 2020

Can we put all the source files from this PRs, those that you enhanced the XML comments, in a separate PR?

Because I think that's independent of which doc build we use.

I will also reduce the amount of code to review here.

 * Copy current template from fsdocs to docsr/_template.html
 * Move docsrc/content and docsrc/files to root of docsrc
 * Update paths to suit that move
 * Remove unused docsrc/tools/templates
@wallymathieu
Copy link
Member

I'm thinking of upgrading the doc generation in this PR #372 so that we can use the way nicer template you have made and the documentation improvements without losing the ability to modify some of the expressions.

@adz
Copy link
Contributor Author

adz commented Oct 10, 2020

@gusty I removed the xml doc changes, but note that most of the changes are just "../../doc" to "../doc" since fsdocs expects the docs to mirror the final path (i.e. not under content or files).

@wallymathieu happy for you to do that -- but note that if you upgrade and use fsdocs there are the path changes I mentioned. We might have to combine the changes in this PR at the same time, or the docs will build with the folder in the path (eg: "https://fsprojects.github.io/FSharpPlus/content/operators-common.html" - note the 'content').

@wallymathieu
Copy link
Member

Yes, I'm trying to dive a bit into F# Formatting command line tool in order to get an idea how it works.

@gusty
Copy link
Member

gusty commented Oct 11, 2020

@gusty I removed the xml doc changes, but note that most of the changes are just "../../doc" to "../doc" since fsdocs expects the docs to mirror the final path (i.e. not under content or files).

No worries, I'm fine with that.

@wallymathieu
Copy link
Member

wallymathieu commented Oct 11, 2020

I have a finished PR with upgrade of the infrastructure and github pages deployment code so that we can turn this PR into a PR for the _template.html file and such.

Copy link
Member

@wallymathieu wallymathieu left a comment

Choose a reason for hiding this comment

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

Taking the weed wacker out can be useful, but I think we lost more that we bargained for

<li class="nav-item" id="fsdocs-repository-link"><a class="nav-link" href="{{fsdocs-repository-link}}">Source Repository</a></li>

<!-- Do not use 'fsdocs-list-of-documents' which currently cannot be ordered, and dumps all files in docsrc/ -->
<li class="nav-header" style="padding-top: 10px">Documentation</li>
Copy link
Member

Choose a reason for hiding this comment

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

This looks nice 👍

@@ -1,7 +1,7 @@
(*** hide ***)
// This block of code is omitted in the generated HTML documentation. Use
// it to define helpers that you do not want to show in the documentation.
#I "../../bin"
#I "../bin"
Copy link
Member

Choose a reason for hiding this comment

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

You can specify input to the tool in order to have another folder than the default.

@@ -136,7 +136,7 @@ For a type defined in an external library it will work when it contains a static

Here's an example of the generic function ``fromBigInt`` targeting a type defined in the MathNet library
*)
#r "../../packages/docs/MathNet.Numerics/lib/net40/MathNet.Numerics.dll"
#r "../../packages/docs/MathNet.Numerics.FSharp/lib/net45/MathNet.Numerics.FSharp.dll"
#r "../packages/docs/MathNet.Numerics/lib/net40/MathNet.Numerics.dll"
Copy link
Member

Choose a reason for hiding this comment

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

these are added through download nuget scripts, so by deleting the tools folder you will need to download these another way.

@@ -1,142 +0,0 @@
module Generate
Copy link
Member

Choose a reason for hiding this comment

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

from what we can see in the other PR, this file is no longer needed

@@ -1,191 +0,0 @@
// Learn more about F# at http://fsharp.org
Copy link
Member

Choose a reason for hiding this comment

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

the code here is needed (as seen in the other PR), but could be turned into a FAKE script

@@ -1,1247 +0,0 @@
module DocLib
Copy link
Member

Choose a reason for hiding this comment

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

Note this is a dumbed down version of FAKE v4

& $nuget install FSharp.Core "-ExcludeVersion" "-version" 4.6.2 "-source" https://www.nuget.org/api/v2 "-OutputDirectory" packages/docs/
& $nuget install System.Runtime "-ExcludeVersion" "-version" 4.3.1 "-source" https://www.nuget.org/api/v2 "-OutputDirectory" packages/docs/
& $nuget install MathNet.Numerics.FSharp "-ExcludeVersion" "-version" 4.8.1 -source https://www.nuget.org/api/v2 "-OutputDirectory" packages/docs/
& $nuget install FSharp.Literate "-ExcludeVersion" "-version" 4.0.0-alpha03 -source https://www.nuget.org/api/v2 "-OutputDirectory" packages/docs/
Copy link
Member

Choose a reason for hiding this comment

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

I think most of these downloads are not needed anymore, the only ones that are relevant are the ones that are referenced from the scripts.

@@ -1,134 +0,0 @@
@startuml
Copy link
Member

Choose a reason for hiding this comment

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

we still want to keep this image around

@wallymathieu wallymathieu mentioned this pull request Oct 13, 2020
3 tasks
@wallymathieu
Copy link
Member

I've done a PR on F# formatting in order to avoid the workaround: fsprojects/FSharp.Formatting#613

@adz adz closed this Oct 26, 2020
@adz adz deleted the adz-fsdocs branch October 26, 2020 20:13
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.

3 participants