`fsdocs init` command #875

kMutagene · 2023-11-18T15:20:25Z

This PR intends to implement a basic version of #872

additionally, i tried to refactor the way default files and folders are accessed from the build command, because the init command needs essentially the same code to copy the relevant files.

kMutagene · 2023-11-18T19:48:13Z

Allright @nojaf @nhirschey i think this is ready for a first review.

I took #875 as an opportunity to get a little familiar with the repo and how the cli tool works, so if anything is not in the correct place please correct me.

currently, the new fsdocs init command creates this scaffold:

└───docs
    │   Dockerfile
    │   index.md
    │   literate_sample.fsx
    │   Nuget.config
    │   _template.html
    │   _template.ipynb
    │   _template.tex
    │
    └───img
            logo.png

i think the _template.* files should be customizable via a flag, but would love some feedback before digging further.

Also, i included just a simple markdown and literate script file to have something there when you run fsdocs watch, this can also serve as a starting point for creating documentation content.

As the init command must copy-paste some of the files that are also searched in the build command, i refactored that as the InRepoLocations and InPackageLocations classes, so those can be accessed across both commands in the same way.

Happy for any feedback

kMutagene · 2023-11-18T19:50:31Z

Note that this can also be easily used to init fsdocs with a custom template by also adding a fsdocs-theme.css file (see #861 ). we could add a set of curated templates to this repo so people can choose from these e.g. via fsdocs init --theme fslab

nojaf

I think this isn't a bad start but I wasn't able to see it in action yet.

I also wonder if we shouldn't prompt the user a bit more about some questions.

We could use Spectr for this.

I thinking about a flow like:

> We notice that you don't have a Build.Directory.props file, do you want to create one (y/n)?

y

> Who are the authors of this project?
Barry & Dave

We could adjust a lot of the often missing substitutions this way.

Of course, we then need a --yes flag to also provide users a way to scaffold everything with all the defaults.

nojaf · 2023-11-20T08:12:51Z

src/fsdocs-tool/InitCommand.fs

+    member this.Execute() =
+
+        let outputPath = Path.GetFullPath(this.output)
+        let repoRoot = Path.GetFullPath(Path.Combine(outputPath, ".."))


This is a bit of an assumption you are making here. If the user is running dotnet fsdocs init in a nested subfolder, this won't be accurate.
Maybe we can run git rev-parse --show-toplevel first and use Path.GetFullPath(Path.Combine(outputPath, "..")) as fallback.

So if i didn't miss anything, the build/watch commands seem to just take user input as given. Should i adjust the init command to the same behavior?

FSharp.Formatting/src/fsdocs-tool/BuildCommand.fs

Lines 1443 to 1447 in 7707ea0

let rootOutputFolderAsGiven =

if this.output = "" then

if watch then "tmp/watch" else "output"

else

this.output

Hmm, I think repoRoot and rootOutputFolderAsGiven are a little different.
I don't think these need to align.

nojaf · 2023-11-20T08:14:16Z

src/fsdocs-tool/InitCommand.fs

+                  (inRepoLocations.index_md_template, Path.GetFullPath(Path.Combine(initLocations.docs, "index.md")))
+                  (inRepoLocations.literate_sample_template,
+                   Path.GetFullPath(Path.Combine(initLocations.docs, "literate_sample.fsx"))) ]
+                |> List.iter (fun (src, dst) -> File.Copy(src, dst, true))


Hmm, I'm not sure if we should overwrite existing files.

How about a --force flag to opt-in to overwriting files?

Yeah maybe. Do you think this would often be used? Running init twice?

nojaf · 2023-11-20T08:14:45Z

src/fsdocs-tool/InitCommand.fs

+                printfn "check it out by running 'dotnet fsdocs watch' !"
+
+                0
+            with _ as exn ->


Suggested change

with _ as exn ->

with exn ->

nojaf · 2023-11-20T08:16:13Z

src/fsdocs-tool/Options.fs

+    ///
+    /// Note that the path of these files will always be combined with the given `assemblyPath` because the cli tool will query it's own path on runtime via reflection.
+    /// </summary>
+    type InPackageLocations(relAssemblyPath) =


Can use InNugetPackageLocations maybe? I'm struggling to wrap my head around this.

nojaf · 2023-11-20T08:21:25Z

src/fsdocs-tool/InitCommand.fs

+                printfn "Error: %s" exn.Message
+                1
+        else
+            printfn


I got this while trying out the local tool in a different repository:

nojaf · 2023-11-20T08:24:35Z

src/fsdocs-tool/Options.fs

+        member _.RelAssemblyPath = relAssemblyPath
+
+        // default folder locations relative to the assembly path
+        member this.docs = Path.Combine(this.RelAssemblyPath, "docs") |> Path.GetFullPath


Nit: I'm not sure how I feel about the lowercase member names.
I get why you went with them. Maybe be we should use ticks and go for something like

member this.``docs/templates`` = ...

this was my initial approach actually, but i find auto-suggestions in my IDE lacking for these members. A small issue though, i'll adjust this accordingly.

nojaf · 2023-11-20T08:26:16Z

src/fsdocs-tool/Options.fs

+        member this.docs_templates_init = Path.Combine(this.docs_templates, "init") |> Path.GetFullPath
+        member this.docs_img = Path.Combine(this.docs, "img") |> Path.GetFullPath
+        member this.docs_content = Path.Combine(this.docs, "content") |> Path.GetFullPath
+        member this.docs_content_img = Path.Combine(this.docs_content, "img") |> Path.GetFullPath


Also a nit but I like a custom operator when using Path.Combine extensively.
Something like let (</>) a b = Path.Combine(a, b), so we can do this.docs_content </> "img"

nojaf · 2023-11-20T08:28:04Z

src/fsdocs-tool/Options.fs

+    ///
+    /// Note that the path of these files will always be combined with the given `assemblyPath` because the cli tool will query it's own path on runtime via reflection.
+    /// </summary>
+    type InPackageLocations(relAssemblyPath) =


Do we even need two classes like this? They look so similar, if they only differ by the starting folder, we perhaps should extract that logic and reuse this type for both cases.

My exact thoughts initially, but the project does pack the files slightly different into the nuget package than the actual paths in the repo, see here for example:

FSharp.Formatting/src/fsdocs-tool/fsdocs-tool.fsproj

Lines 30 to 32 in 7707ea0

<Content Include="..\..\docs\_template.html" PackagePath="templates" />

<Content Include="..\..\docs\_template.tex" PackagePath="templates" />

<Content Include="..\..\docs\_template.ipynb" PackagePath="templates" />

a "templates" folder does not exist in the repo, but will contain the template files in the nuget package. I guess the reason here is that in the repo, we need an intact docs folder. It just felt wrong putting this on the same class when it could contain paths that do not exist.

nojaf · 2023-11-20T10:24:00Z

src/fsdocs-tool/InitCommand.fs

+            ensureOutputDirs ()
+
+            try
+                [ (inPackageLocations.template_html, initLocations.template_html)


I'm very opposed that we copy the _template.html file by default.
This should only be done when users are interested in creating their custom theme.
Which, in a sense we want to avoid. 90% of our user base should be able to use or customize the modern theme instead of writing one from scratch.

So, I would not include _template.html by default.

Sorry I think i do not really get the difference - when the user does not put a _template.html file into their docs folder, isn't the file i am copying here the exact one that will be used by the tool per default anyways?

My rationale would be that you will need a template file once you want to make even slight changes to the layout, but want to keep most of the default file. Where would i get that file other than looking up the file in the FSharp.Formatting repo?

On the other hand, this could be circumvented by putting a good descriptive interactive text on the setup, so users that will need to make adjustments just opt-in to copy the template file

Oh i think i get the issue. If we update the file for a new version of the tool, the repos that use the copied template file will have an outdated version. So i agree this should be opt-in

Yes, indeed. If the user doesn't have a _template.html, the one from NuGet is used.
Which will stay updated if they bump the version.

nojaf · 2023-11-20T10:25:29Z

src/fsdocs-tool/InitCommand.fs

+                printfn "check it out by running 'dotnet fsdocs watch' !"
+
+                0
+            with _ as exn ->


Suggested change

with _ as exn ->

with exn ->

kMutagene · 2023-11-21T08:21:15Z

Allright a quick summary of how i would proceed:

Make the command interactive by default, letting the user decide what files to use accompanied by a descriptive text of what the files do and in which scenario it makes sense to use them as defaults.
Use the same behavior for output path as the other commands
More descriptive name for InNugetPackageLocations, and adjust member names with double ticks to fully represent the locations
Overall better defaults for the non-interactive variant of the command (no template files)

sounds good @nojaf ?

kMutagene · 2023-11-21T08:23:52Z

I would like it more if there would be a way to unify InPackageLocations and InRepoLocations though, so it could just be DefaultLocations determined from a base path. However i think this would need adjusting the way the templates and extras are packaged into the nuget package, see my other comments

nojaf · 2023-11-21T16:52:07Z

sounds good @nojaf ?

Yup!

…commands

and comment init command code a little

- more descriptive class names - member names now reflect paths exactly - add badge svg file paths - add location descriptions

kMutagene · 2023-11-28T09:22:51Z

Hey @nojaf, where should I put tests that check whether the location classes construct correct paths? None of the test projects seem to match the scope of testing 'Common' functions used across commands in the tool.

nojaf · 2023-11-28T12:40:10Z

Hmm, this does seem like a rather new area when it comes to the existing test projects.
I'm not sure really, you could create a new project I suppose.
On the other hand, will this be easy to write unit tests for?
Could you give an example of what you have in mind you want to test?

kMutagene · 2024-01-02T07:41:30Z

you could create a new project I suppose

This is what i feared, because all the existing test projects look really arcane to me - i usually just use expecto, here it seems to be FsUnit + NUnit - i can roll with that, but every test i looked at also makes heavy use of compiler directives and i have absolutely no clue on what to use from that.

Could you give an example of what you have in mind you want to test?

A really simple case would be verifying that all default paths created from a base directory are correct

nojaf · 2024-01-02T08:35:52Z

Well, the whole solution is rather arcane, isn't it?
Anyway, I don't think all test files are that bad.

You could also consider using https://github.com/TestableIO/System.IO.Abstractions in your implementation. That could make writing such tests easier.

kMutagene · 2024-01-02T12:18:25Z

https://github.com/TestableIO/System.IO.Abstractions

oh wow, that will come in handy in other projects as well, thanks! I'll copy stuff together until i have working tests i guess ;)

kMutagene force-pushed the init-command branch from 9820686 to 3b00528 Compare November 18, 2023 18:04

kMutagene marked this pull request as ready for review November 18, 2023 19:48

nojaf mentioned this pull request Nov 20, 2023

Modern theme #861

Merged

nojaf reviewed Nov 20, 2023

View reviewed changes

kMutagene added 7 commits November 27, 2023 10:19

add attempt to make in-repo and in-package locations reusable across …

ffd330a

…commands

Further refactoring of build command using new locations

6a85a1b

add init command template files and add them to location abstractions

40fa1e0

add first basic implementation of the init command

3024e00

typo fix in literate script sample

7a4f227

add cli success messages

f5bd211

and comment init command code a little

improve default location abstractions

e28ee79

- more descriptive class names - member names now reflect paths exactly - add badge svg file paths - add location descriptions

kMutagene force-pushed the init-command branch from 2f9de2c to e28ee79 Compare November 28, 2023 08:37

Add basic interactivity for fsdocs init

01a5567

kMutagene marked this pull request as draft November 28, 2023 09:26

add non-interactive flag to init command

6cea970

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fsdocs init` command #875

`fsdocs init` command #875

kMutagene commented Nov 18, 2023

kMutagene commented Nov 18, 2023 •

edited

Loading

kMutagene commented Nov 18, 2023

nojaf left a comment

nojaf Nov 20, 2023

kMutagene Nov 21, 2023

nojaf Nov 21, 2023

nojaf Nov 20, 2023

kMutagene Nov 21, 2023

nojaf Nov 21, 2023

nojaf Nov 20, 2023

nojaf Nov 20, 2023

nojaf Nov 20, 2023

nojaf Nov 20, 2023

kMutagene Nov 21, 2023

nojaf Nov 20, 2023

nojaf Nov 20, 2023

kMutagene Nov 21, 2023

nojaf Nov 20, 2023

kMutagene Nov 21, 2023

kMutagene Nov 21, 2023 •

edited

Loading

kMutagene Nov 21, 2023

nojaf Nov 21, 2023

nojaf Nov 20, 2023

kMutagene commented Nov 21, 2023 •

edited

Loading

kMutagene commented Nov 21, 2023

nojaf commented Nov 21, 2023

kMutagene commented Nov 28, 2023

nojaf commented Nov 28, 2023

kMutagene commented Jan 2, 2024

nojaf commented Jan 2, 2024

kMutagene commented Jan 2, 2024

	let rootOutputFolderAsGiven =
	if this.output = "" then
	if watch then "tmp/watch" else "output"
	else
	this.output

	<Content Include="..\..\docs\_template.html" PackagePath="templates" />
	<Content Include="..\..\docs\_template.tex" PackagePath="templates" />
	<Content Include="..\..\docs\_template.ipynb" PackagePath="templates" />

fsdocs init command #875

Are you sure you want to change the base?

fsdocs init command #875

Conversation

kMutagene commented Nov 18, 2023

kMutagene commented Nov 18, 2023 • edited Loading

kMutagene commented Nov 18, 2023

nojaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kMutagene Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kMutagene commented Nov 21, 2023 • edited Loading

kMutagene commented Nov 21, 2023

nojaf commented Nov 21, 2023

kMutagene commented Nov 28, 2023

nojaf commented Nov 28, 2023

kMutagene commented Jan 2, 2024

nojaf commented Jan 2, 2024

kMutagene commented Jan 2, 2024

`fsdocs init` command #875

`fsdocs init` command #875

kMutagene commented Nov 18, 2023 •

edited

Loading

kMutagene Nov 21, 2023 •

edited

Loading

kMutagene commented Nov 21, 2023 •

edited

Loading