-
Notifications
You must be signed in to change notification settings - Fork 74
Fix excluded files and folder deletion triggers #112
Conversation
{ | ||
filesToWatch.AddRange(compilerOptions.CompileInclude.ResolveFiles()); | ||
|
||
if (compilerOptions.EmbedInclude != null) |
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.
Should this be nested?
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.
That's a good question, maybe you have an answer. So this new project model has two sets of properties: the old ones and the new ones.
- Can you have both of them set at the same time?
- Does having one set mean ignore the other one?
- Which one takes priority because I see that the new properties are
null
by default if not set but the old ones have default values if not set
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.
Can you have both of them set at the same time? Does having one set mean ignore the other one?
Yes. The new ones will take priority.
Which one takes priority because I see that the new properties are null by default if not set but the old ones have default values if not set
When nothing is set, the default of old schema is used.
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.
In this case, if CompileInclude
is not set but EmbedInclude
is set it will be ignored.
This should probably be refactored to something like this.
if (compilerOptions.CompileInclude != null)
{
filesToWatch.AddRange(compilerOptions.CompileInclude.ResolveFiles());
}
else
{
filesToWatch.AddRange(runtimeProject.Files.SourceFiles);
}
if (compilerOptions.EmbedInclude != null)
{
filesToWatch.AddRange(compilerOptions.EmbedInclude.ResolveFiles());
}
else
{
filesToWatch.AddRange(runtimeProject.Files.ResourceFiles.Values);
}
filesToWatch.AddRange(runtimeProject.Files.PreprocessSourceFiles);
filesToWatch.AddRange(runtimeProject.Files.SharedFiles);
⌚ |
runtimeProject.Files.SharedFiles))).Concat( | ||
new string[] { runtimeProject.ProjectFilePath }) | ||
.ToList(); | ||
var compilerOptions = runtimeProject.GetCompilerOptions(targetFramework: null, configurationName: null); |
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.
Can you have files included/excluded per configuration/framework?
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.
@ajaybhargavb this is a question for you
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.
compile
and embed
are part of buildOptions
which can be framework/configuration specific. So yes the schema supports it. But I am not sure if it makes sense to include/exclude files for specific framework/configuration.
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 think there are rare scenarios like that but I am not sure if we need to proactively address them now.
63704e0
to
85f4c39
Compare
Updated |
from me |
var compilerOptions = runtimeProject.GetCompilerOptions(targetFramework: null, configurationName: null); | ||
|
||
var filesToWatch = new List<string>() { runtimeProject.ProjectFilePath }; | ||
if (compilerOptions?.CompileInclude != null) |
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.
Sorry if my reply was confusing before, I meant that compilerOptions
will never be null.
- Bugfix: Trigger on folder deletion - Add test for folder deletion
85f4c39
to
2cf772b
Compare
Fixes #90, https://github.com/aspnet/dotnet-watch/issues/93
Please review @moozzyk @kichalla @ajaybhargavb