GH2140: PR DotNetCorePublish does not respect SelfContained DotNetCorePublishSettings property #2140 #2142
Conversation
|
|
| } | ||
| else | ||
| { | ||
| builder.Append("--self-contained false"); |
There was a problem hiding this comment.
Can we divide this into two separate appends instead?
There was a problem hiding this comment.
You mean two separate ifs, one for true and another if for false?
There was a problem hiding this comment.
No, like this:
builder.Append("--self-contained");
builder.Append("false");
|
@kcamp Correct, but I think the impact will be minimal from changing this property (especially since it hasn't worked properly before). |
|
No prob. Just trying to avoid a repeat. 👍 |
|
@kcamp is this a breaking change, it's not working as is.... This wouldn't create a regression at all? |
There was a problem hiding this comment.
I could also rewrite a bit differently, less code but maybe less readable....
From:
if (settings.SelfContained.HasValue)
{
if (settings.SelfContained.Value)
{
builder.Append("--self-contained");
}
else
{
builder.Append("--self-contained");
builder.Append("false");
}
}
To:
if (settings.SelfContained.HasValue)
{
builder.Append("--self-contained");
if (!settings.SelfContained.Value)
{
builder.Append("false");
}
}
I prefer the current version, it feels more clear/readable to me....
|
@klabranche Yeah, I think the current version is more readable as well 👍 |
|
Always nice to see every build fail.... Anything I can do @patriksvensson? Assuming not.... |
|
@klabranche It looks like some extra whitespace (line break) in the style analysis. You should be able to check build output via TC by logging in as guest. |
|
OH.... didn't know that but also read the contribs and didn't realize I had some extra lines... my bad.... |
|
All good to go it looks like. :-) ? |
devlead
changed the title
since no setting means it takes the .Net default of true when runtime is included. Added the logic to include false for self contain and test Fixes #2140
|
@klabranche your changes have been merged, thanks for your contribution 👍 |
Made SelfContainedproperty of DotNetPublishSettings a nullable boolean since no setting means it takes…the .Net default of true when runtime is included. Added the logic to include false for self contain and test when running DotNetPublish.
#2140