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

Split up FSharp.Data #1457

Merged
merged 11 commits into from
Aug 16, 2022
Merged

Split up FSharp.Data #1457

merged 11 commits into from
Aug 16, 2022

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Aug 15, 2022

@cartermp We've discussed splitting out core packages for Http, Csv etc.

The split in this PR is this:

  • FSharp.Data -- everything including type providers
    • FSharp.Data.Http -- http helpers
    • FSharp.Data.Csv.Core -- csv types/helpers
    • FSharp.Data.Json.Core -- json types/helpers
    • FSharp.Data.Html.Core -- html types/helpers
    • FSharp.Data.Xml.Core -- xml types/helpersThis attempts to do that.
    • FSharp.Data.WorldBank.Core -- xml types/helpersThis attempts to do that.

At a later point we may split out the separate type providers to allow you to use just the one you want. However that's not done here - currently if you want a type provider you reference FSharp.Data which implies all the core pacakges too.

Some of the design-time inference logic used by the type providers is in the Core packages, now made internal.

Some of the public IO and pluralization helpers used by the the other components and the type provider inference are in FSharp.Data.Http. These may be moved around or made internal in a future release. There's no "right" place to put these.

@cartermp
Copy link
Collaborator

What's the benefit to pulling it out to be a separate dll?

@dsyme
Copy link
Contributor Author

dsyme commented Aug 15, 2022

@cartermp The relevant issue is here: #1311

We can decide on the split we want. To me the following eventually makes sense, as it's reasonable to just want the helpers for a particular data format:

  • FSharp.Data -- includes all type provider references
  • FSharp.Data.Core -- causes no type provider references
    • FSharp.Data.Http.Core -- causes no type provider references
    • FSharp.Data.Csv.Core -- causes no type provider references
    • FSharp.Data.Json.Core -- causes no type provider references
    • FSharp.Data.Html.Core -- causes no type provider references
    • FSharp.Data.Xml.Core -- causes no type provider references
    • FSharp.Data.WorldBank.Core -- causes no type provider references

What's currently in this PR is this, which is a stepping stone to the above:

  • FSharp.Data -- includes type provider references
  • FSharp.Data.Core -- causes no type provider references, just the various data formats and helpers

I don't think it's worth splitting the different type providers out as yet - at least it's a lot of work as each also needs a DesignTime thing. But we should probably leave the door open to doing that.

@dsyme
Copy link
Contributor Author

dsyme commented Aug 15, 2022

Maybe a better alternative is this:

  • FSharp.Data -- includes everything
    • FSharp.Data.Http -- http helpers
    • FSharp.Data.Csv -- csv types/helpers and CsvProvider
      • FSharp.Data.Csv.Core -- csv types/helpers
    • FSharp.Data.Json -- json types/helpers + JsonProvider
      • FSharp.Data.Json.Core -- json types/helpers
    • FSharp.Data.Html -- html types/helpers + HtmlProvider
      • FSharp.Data.Html.Core -- html types/helpers
    • FSharp.Data.Xml -- xml types/helpers + XmlProvider + XsdProvider
      • FSharp.Data.Xml.Core -- xml types/helpers
    • FSharp.Data.WorldBank -- world bank thing

This is based on the logic that

  • The most common thing is "just give me the support for X, including everything"
  • Occasionally, people want "just give me the core helpers for X, forget the type provider"

That does mean splitting the specific type provider DesignTime out, it's work though the user doesn't see that.

A stepping stone could be:

  • FSharp.Data -- includes everything
    • FSharp.Data.Http -- http helpers
    • FSharp.Data.Csv.Core -- csv types/helpers
    • FSharp.Data.Json.Core -- json types/helpers
    • FSharp.Data.Html.Core -- html types/helpers
    • FSharp.Data.Xml.Core -- xml types/helpers

@dsyme dsyme changed the title split out FSharp.Data.Core [WIP] split up FSharp.Data Aug 15, 2022
@pimbrouwers
Copy link

Done and done!

Screenshot_20220815-184700

@cartermp
Copy link
Collaborator

@dsyme makes sense!

@@ -141,15 +141,16 @@ Target.create "NuGet" (fun _ ->
("PublishRepositoryUrl", "true")
("EmbedUntrackedSources", "true")
("IncludeSymbols", "true")
("SymbolPackageFormat", "snupkg") ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Contributor Author

@dsyme dsyme Aug 16, 2022

Choose a reason for hiding this comment

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

So I can't work out why having this in fails the package build when there are two packages.

Without it we get FSharp.Data.Core.symbols.nupkg and FSharp.Data.symbols.nupkg correctly built but the file name extensions are different

Do you know if it matters either way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://docs.microsoft.com/en-us/nuget/create-packages/symbol-packages-snupkg

The legacy format .symbols.nupkg is still supported but only for compatibility reasons like native packages (see Legacy Symbol Packages). NuGet.org's symbol server only accepts the new symbol package format - .snupkg.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just embed symbols instead of the separate package? That's much less hassle for consumers. Separate packages make Sourcelink hard to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baronfel Thanks, I've done this now

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

@dsyme
Copy link
Contributor Author

dsyme commented Aug 16, 2022

I've updated the PR to split out the core pacakges

@dsyme dsyme changed the title [WIP] split up FSharp.Data Split up FSharp.Data Aug 16, 2022
@dsyme
Copy link
Contributor Author

dsyme commented Aug 16, 2022

@cartermp @baronfel This is ready (once green, just niggles with doc gen)

Copy link
Collaborator

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks clean to me (pending docs changes)

@dsyme dsyme merged commit cdd5533 into fsprojects:main Aug 16, 2022
@dsyme
Copy link
Contributor Author

dsyme commented Aug 17, 2022

This is now merged and released as FSharp.Data 5.0.1

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.

4 participants