-
Notifications
You must be signed in to change notification settings - Fork 830
F# Scripts: Fix resolving the dotnet host path when an SDK directory is specified #18960
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
base: main
Are you sure you want to change the base?
F# Scripts: Fix resolving the dotnet host path when an SDK directory is specified #18960
Conversation
❗ Release notes required
|
let n = arguments.Length | ||
|
||
seq { for i in n - 1 .. -1 .. 0 -> arguments[0..i] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the intention.
But still, I believe future human and non-human coders would appreciate a few comments about what is being done here and especially why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changes, docs or implementation or some limited guidance perhaps, are to be done towards 3rd party Dependency Providers together with this change?
In this case, the purpose (fixing nuget, the dominant provider) is justified and I think it's OK to expand the possibility of sdkDirOverride
to other providers - do I understand it right that now every provider can retrieve this info?
(I am coming from a philosophical question if a direct string
param is correct here and if it does not close too many future-expansion points.
After all, the construction logic is driven via constructor overloads and locking down string option
as a constructor argument might be kick as back later.
Have you considered ways of making this parameter more semantic, either by changing construction logic or by also utilizing the parameter's name on top of the GetConstructor search perhaps?)
Right.
I've been looking at similar changes with adding the result caching parameter, but I didn't see any docs change there (if don't count the description for an fsi parameter). The readme does not contain additional information about the parameters and simply refers to the default implementation:
Nevertheless,
The current dependency provider plugin system is based on duck typing and versioning via constructor overloads. The main thing here is the signature of the constructor, and no one is obligated to use the exact names of the parameters; I also do not know if there are third-party implementations that use not strict naming for these parameters. One of the options I considered was using a public property with a setter:
Perhaps we could use this approach here. We can also make everything even more fluid and pass Do you have better ideas? |
Thanks @DedSec256 . Since it is all invoked via reflection anyway, I do not see that much of a difference between properties (which will be authored as strongly-typed, but in practice always set by reflection) and a dictionary property bag. Right now, the |
The
FSharpChecker.GetProjectOptionsFromScript
method has an optional parametersdkDirOverride
, which allows specifying the SDK with which the script will be analyzed in the IDE. However, for restoring NuGet packages, this info is not used, which leads to some unpredictable behavior. In some cases, for example, when using a new version of .NET installed in a non-standard location, restoring NuGet packages becomes impossible with an error likeThis PR fixes this issue, taking into account the
sdkDirOverride
parameter for NuGet restoring in the defaultFSharpDependencyManager
.It also improves additional log info in the generated
Project.fsproj.fsx
, showing the correct path to the useddotnet.exe
instead of the outdated hardcoded.