nhirschey / FSharp.Data.Fred

Wrapper for Federal Reserve Economic Data API https://nhirschey.github.io/FSharp.Data.Fred/
MIT License
2 stars 3 forks source link

support for netstandard2.0 #17

Closed smoothdeveloper closed 11 months ago

smoothdeveloper commented 11 months ago

This should enable using the library with .NET framework targets as well.

Went over the unit tests (with api key and developer mode) and built the doc again to verify things are still OK.

smoothdeveloper commented 11 months ago

remark: setting as work in progress as I'm facing some issue with transitive FSharp.Data dependency when running samples under fsianycpu.exe (the .NET Framework version of FSI).

smoothdeveloper commented 11 months ago

It is actually working, it is just necessary to reference the same version of FSharp.Data (4.2.4 as of now) in order for things to bind correctly.

I think this can be merged, it works with .NET Framework FSI now.

nhirschey commented 11 months ago

I'm facing some issue with transitive FSharp.Data dependency when running samples under fsianycpu.exe (the .NET Framework version of FSI).

I don't recall if there is an issue using netstandard or not. There's weirdness using FSharp.Data as a Library for parsing Json as we are here. I thought targetting net5.0/6.0 was because of a prior FAKE dependency, but maybe there are issues.

FYI I pushed a commit fixing the github action workflow workflow.

smoothdeveloper commented 11 months ago

I don't recall if there is an issue using netstandard or not. There's weirdness using FSharp.Data as a Library for parsing Json as we are here. I thought targetting net5.0/6.0 was because of a prior FAKE dependency, but maybe there are issues.

I was thinking to make a separate PR to remove FAKE dependencies from paket.dependencies (and running paket install again), do you want me to adjust it in this PR?

The error I'm facing is, I think, because FSharp.Data split the types in separate assembly, and as of 4.2.4 it still expects everything under a single assembly.

We should focus on upgrading FSharp.Data to the earliest version that introduced the split, but this would require a bit of research.

FYI I pushed a commit fixing the github action workflow workflow.

No problem!

nhirschey commented 11 months ago

it is just necessary to reference the same version of FSharp.Data (4.2.4 as of now)

I think #r nuget doesn't pick up dependency restrictions properly (I've had this issue elsewhere). You have to manually specify versions.

Anyway, thanks for putting this together!

smoothdeveloper commented 11 months ago

Confirming it works with 4.2.10, so it doesn't have to be the exact same version, just the types have to be in same assembly as the library is compiled for.

The earlier version of FSharp.Data that bring the split seems to be 5.0.1: https://www.nuget.org/packages/FSharp.Data.Json.Core/#versions-body-tab

So my suggestion, on a next major update is to update to 5.0.1, and people should be able to use any later release, it would simplify the `#r "nuget: FSharp.Data" to not have to bake the version.

nhirschey commented 11 months ago

I was thinking to make a separate PR to remove FAKE dependencies from paket.dependencies (and running paket install again), do you want me to adjust it in this PR?

It would be excellent if you removed them here.

The error I'm facing is, I think, because FSharp.Data split the types in separate assembly, and as of 4.2.4 it still expects everything under a single assembly.

We should focus on upgrading FSharp.Data to the earliest version that introduced the split, but this would require a bit of research.

About that time they merged a breaking change to HTTP/Json parsing which is the problem. Phillip knew it was breaking when he merged it but our usage here is a bit of an edge case.

I think 5.0.2 is the last compatible FSharp.Data before the break. Happy for you to update it in this PR if you'd like, but I'm happy to merge without this change.

smoothdeveloper commented 11 months ago

It seems non trivial to update it without breaking documentation generation for now, I'll try to pay a look at another time.

5.0.2 seems to be same as the 4.2.10 based on the release note, best for now is to keep it as it is.

nhirschey commented 11 months ago

Thank you @smoothdeveloper for providing this improvement! It should show up on nuget soon as v0.2.0.

smoothdeveloper commented 11 months ago

Great, will be able to use it out of box off nuget then :)

Thanks for the great package!