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

feat: lower FSharp.Core requirements #7

Merged

Conversation

MangelMaxime
Copy link
Contributor

Hello @cmeeren,

This PR is a suggestion I choose to make it via a PR instead of an issue because it is easier to showcase the impacted code.

I am currently exploring writing test for .NET and found your library. While giving it a try, I had this warning:

warning NU1605: Detected package downgrade: FSharp.Core from 7.0.400 to 5.0.2. Reference the package directly from the project to select a different version. 
warning NU1605:  Ionide.XmlDocFormatter.Tests -> Faqt 4.0.0 -> FSharp.Core (>= 7.0.400) 
warning NU1605:  Ionide.XmlDocFormatter.Tests -> FSharp.Core (>= 5.0.2)

This is happening because when creating a library I make FSharp.Core requirements as low as possible and in general I use FSharp.Core 5.0.2 because it has most of the goodies that are needed now days.

I am following the recommendation from F# Compiler Guide:

As an F# package author, you also need to make this decision with respect to FSharp.Core:

  • Targeting an earlier version of FSharp.Core increases your reach because older codebases can use it without issue
  • Targeting a newer version of FSharp.Core lets you use and extend newer features

This decision is critical, because it can have a network effect. If you choose a higher FSharp.Core version, then that also becomes a dependency for any other package that may depend on your package.

In my projects, I am using central package management which allows to handle versions for the packages at a single place instead of having to update each project individually. Because of these 2 reasons, I am getting the warning mentioned above when adding Faqt.

While looking at Faqt code, I discovered that there was only 1 place which needed a more recent version of FSharp.Core so I think it can be good to make the proposed change. It also means that Faqt can be used by a wider set of project without requirement then to depends on a newer version of FSharp.Core.

@cmeeren
Copy link
Owner

cmeeren commented Aug 24, 2024

Thanks! This sounds good. Could you change the code to the following?

if countsLeft |> Map.forall (fun _ v -> v = 0) then

@MangelMaxime MangelMaxime force-pushed the feature/low_fsharp_core_requirements branch from aa267dc to a4877c2 Compare August 24, 2024 14:55
@MangelMaxime
Copy link
Contributor Author

@cmeeren I made the changes

@cmeeren cmeeren merged commit 6b5c511 into cmeeren:main Aug 24, 2024
8 checks passed
@cmeeren
Copy link
Owner

cmeeren commented Aug 24, 2024

Releasing shortly in v4.0.1. Thanks again!

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.

2 participants