-
Notifications
You must be signed in to change notification settings - Fork 420
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
Bump Dotnet.Script.DependencyModel to version 0.50.0 #1609
Conversation
<ItemGroup> | ||
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0-beta2-18618-05" PrivateAssets="All" /> | ||
</ItemGroup> | ||
<Project ToolsVersion="14.0" |
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.
could you please reset the changes to this file so that only the relevant lines are updated? we often look here at the blame to identify when a specific dependency was introduced - and this will break it
@@ -66,8 +69,9 @@ public ScriptContext CreateScriptContext(ScriptOptions scriptOptions) | |||
CompilationDependency[] compilationDependencies = null; | |||
try | |||
{ | |||
var allCsxFiles = _fileSystemHelper.GetFiles("**/*.csx").ToArray(); |
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.
the same files are already discovered in ScriptProjectSystem; since this is potentially an expensive operation at start up, we should not do it twice. can we pass a list of files here instead?
_scriptOptions = new ScriptOptions(); | ||
_scriptOptions.CsxFiles = allCsxFiles; |
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.
actually it would be better to just add an extra argument to CreateScriptContext(...)
.
ScriptOptions
is bound from the omnisharp.json
so it's not good idea to use it for just transporting objects or data from one place to another
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.
LGTM - thanks!
. Net Core 3.0 compilation deps needs more work |
@mholo65 any thoughts here? |
@@ -92,7 +111,13 @@ public ScriptContext CreateScriptContext(ScriptOptions scriptOptions) | |||
isDesktopClr = false; | |||
HashSet<string> loadedFiles = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | |||
|
|||
foreach (var compilationAssembly in compilationDependencies.SelectMany(cd => cd.AssemblyPaths).Distinct()) | |||
// Pick the highest version | |||
var resolvedAssemblyPaths = compilationDependencies.SelectMany(cd => cd.AssemblyPaths) |
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.
"one liner" 😂
@filipw yeah I'm fine with nuget 5.2, we need to stay more up to date on these things anyway. |
This PR bumps the
Dotnet.Script.DependencyModel
andDotnet.Script.DependencyModel.NuGet
packages to version0.50.0
adding support for .Net Core 3.0 based scripts.