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

Expand documentation #100

Merged
merged 15 commits into from
Nov 6, 2021
Merged

Expand documentation #100

merged 15 commits into from
Nov 6, 2021

Conversation

Gastove
Copy link
Contributor

@Gastove Gastove commented Jan 23, 2021

Note: to get everything working nicely, this branch was opened off of my branch for #97. It will be much easier to review this PR after #97 has closed, as it contains all of 97s commits!

This PR proposes some expansions to Fleece's documentation. In particular, my goal is to cover sets of use-cases I found non-obvious as a newcomer to Fleece. I've tried to clarify a few points around working with DUs, as well as dealing with fields that are required in a given JSON object but not in F#.

Per #98, I've also bumped the SDK version in the project global.json to 5.0, to allow the use of F#5's package-loading features in documentation scripts.

I am not, for the record, adamant about any of this. Fleece is a great project, and I'd love to help! And, I'm pretty happy to take a different approach.

@Gastove
Copy link
Contributor Author

Gastove commented Jan 23, 2021

Huh. I just realized I never bumped the actual dotnet versions being used for CI. But -- CI is perfectly happy, so it certainly doesn't seem to be harming anything. If the global.json change works for y'all, I'm very happy to make sure CI stays in sync as part of this PR. Just let me know!

@wallymathieu
Copy link
Member

I think it makes sense to upgrade in order to be able to use this new feature 👍. I think you can also remove the:

  • docsrc/tool/download_nugets.ps1
  • docsrc/tool/download_nugets.sh

since these download needed nuget packages into folders, while if you use the new F# 5 nuget reference syntax these are unnecessary.

@wallymathieu
Copy link
Member

Note for instance:

https://github.com/fsprojects/Fleece/blob/master/docsrc/tool/generate.fs#L52-L73

let docPackagePath  path =
    __SOURCE_DIRECTORY__ + @"/../../packages/docs/" + path
...
let evaluationOptions = 
    [| 
         includeDir "FSharp.Core/lib/netstandard2.0/"
         reference "FSharp.Core/lib/netstandard2.0/FSharp.Core.dll" 
         includeDir "FSharp.Literate/lib/netstandard2.0/" 
         includeDir "MathNet.Numerics/lib/netstandard2.0/" 
         reference "MathNet.Numerics/lib/netstandard2.0/MathNet.Numerics.dll" 
         includeDir "MathNet.Numerics.FSharp/lib/netstandard2.0/" 
         reference "MathNet.Numerics.FSharp/lib/netstandard2.0/MathNet.Numerics.FSharp.dll" 
         includeDir "System.Buffers/lib/netstandard2.0/" 
         includeDir "System.Collections.Immutable/lib/netstandard2.0/" 
         includeDir "System.Reflection.Metadata/lib/netstandard2.0/" 
         includeDir "System.ValueTuple/lib/netstandard1.0/" 
         reference "System.ValueTuple/lib/netstandard1.0/System.ValueTuple.dll" 
         includeDir "FSharp.Compiler.Service/lib/netstandard2.0/" 
         reference "FSharp.Compiler.Service/lib/netstandard2.0/FSharp.Compiler.Service.dll" |] 

you could use F#5 NuGet reference instead of using NuGet to download packages. I think that it would then only be necessary to reference Suave, TaskBuilder.fs et.c. directly in the documentation.

@wallymathieu
Copy link
Member

In order to test out such changes you could do:

dotnet tool restore
dotnet build -c Release
dotnet run --project ./docsrc/tool

@wallymathieu
Copy link
Member

Though perhaps I (or someone willing) should do a separate pull request (once you are finished with this one) with those changes since I think the scope you have for this is to expand the documentation.

@wallymathieu
Copy link
Member

Since the PR #102 has been merged, is there anything you want to change in this PR?

@Gastove
Copy link
Contributor Author

Gastove commented Apr 25, 2021

Ha, I lost track of this. Will review, fix conflicts, etc.

This commit is one possible solution to fsprojects#96. Here, I'm simply dropping support
for net461 entirely, in favor of targeting `netstandard`. While I was doing
this, I bumped the `netstandard` version to 2.1, but this could easily be an
addition instead of a replacement. Similarly, I noted that the `global.json`
specified SDK version 3.0, but with a roll-forward directive for newest minor
version. Accordingly, I bumped to the newest SDK 3.1 release. These changes
meant I could remove the `mono` block from the Fleece solution, so I did.

These changes reach all through Fleece, and prompted the replacement of `net461`
with `netstandard2.1` (mostly in documentation) or `netcoreapp3.1` (mostly in
tests) in many places. While I was doing this, I cleaned whitespace and
indentation, and added a missing XML specifier to an fsproj file.

On my machine, the build and all tests are passing.
This commit:

- Expands the Codec section for DUs to cover non-data-bearing DUs and DUs with
type-tags.
- Adds a new `further-techniques` doc to cover cases like fields that are
required in JSON but not in an F# data model.
- Updates the index.
- Updates the global.json to SDK version 5.0 to allow F#5 scripting syntax.
- Closes fsprojects#98
@Gastove
Copy link
Contributor Author

Gastove commented Apr 25, 2021

Okay, that should at least be properly rebased and cleaned up. I had a rather alarming editor snafu, so that was a bit messier than expected, but I think it looks OK now.

I think the last thing I'd love y'all's guidance on is: using released library code versus compiled local assemblies in the docs. Using released library code is considerably more ergonomic; a human contributor doesn't have to remember to do anything before the docs load correctly in an editor. On the other hand, this means there isn't actually exact parity between the docs and the code they document, which doesn't seem great.

A further challenge: when referencing local assemblies, if one updates a dependency version for a particular library, all documentation paths must be updated to the same version. This does enforce correctness, and is just frustrating.

My current thought is:

  • Nuget reference syntax with versions for external dependencies. (Makes it, if nothing else, easier to spot and update the versions.)
  • Local DLL reference for Fleece itself.
  • Documentation making it clear to run a build before loading doc fsx files.

Thoughts from y'all maintainers? What do you prefer?

@Gastove
Copy link
Contributor Author

Gastove commented Apr 25, 2021

(Whatever we decide, I'll write some documentation about how to write/contribute documentation and add it to this PR, make sure it's captured clearly.)

@wallymathieu
Copy link
Member

I think it makes sense to compile the documentation against the source, while having F# 5 NuGet references visible in the docs (for instance via markdown code includes in comments). Though having the docs compile against a released version would imply a separate life cycle of the docs compared compared to the lib: Since you'd have to document new features after a finished release. The reason to tie docs with locally compiled binaries is that you could start document unreleased features.

@wallymathieu
Copy link
Member

I'm thinking to adjust this PR slightly in order to merge and push documentation. I'll see when I have the time.

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.

👍

@wallymathieu
Copy link
Member

I've changed the ShapeD codec definition slightly, what's your thoughts @Gastove ?

@wallymathieu
Copy link
Member

I'm tinking of fixing the issues in order to be able to merge this PR @Gastove

Copy link
Member

@gusty gusty left a comment

Choose a reason for hiding this comment

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

Approved with suggestions: we have to deprecate some stuff, but we can do it later, in the PR for Fleece vNext.

docsrc/content/further-techniques.fsx Outdated Show resolved Hide resolved
|> withFields
|> jfieldOpt "result" (fun r -> r.Result)
|> jfieldOpt "error" (fun r -> r.Error)
|> jfield "jsonrpc" (fun _ -> "2.0")
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 we should start deprecating this verbose syntax, in favor of coming Applicative Computation Expressions in Fleece vNext.

For the moment I would use the applicative syntax with operators.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Should we include it in the docs and write that it's obsolete then?

docsrc/content/codec.fsx Outdated Show resolved Hide resolved
docsrc/content/combinators.fsx Outdated Show resolved Hide resolved
docsrc/content/comparison-with-json-net.fsx Outdated Show resolved Hide resolved
@gusty gusty merged commit 6158ce1 into fsprojects:master Nov 6, 2021
@wallymathieu
Copy link
Member

Many thanks @Gastove for the contribution!

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.

None yet

3 participants