Change default workload set use to 'true' #45936
Conversation
ghost
added
| @@ -40,6 +40,8 @@ public override string ToString() | |||
| { | |||
| return JsonSerializer.Serialize<InstallStateContents>(this, s_options); | |||
| } | |||
|
|
|||
| public bool ShouldUseWorkloadSets() => UseWorkloadSets ?? true; | |||
There was a problem hiding this comment.
excellent idea - nitpick though - should use workload sets be hidden in some way so that we don't use it accidentally in the codebase, instead always going through your new helper?
There was a problem hiding this comment.
Unfortunately, JsonSerializer doesn't serialize private properties without a good bit of extra work:
https://stackoverflow.com/questions/61869393/get-net-core-jsonserializer-to-serialize-private-members
Good idea, though 🙂
There was a problem hiding this comment.
crazier idea - maybe add the [EditorBrowsable(EditorBrowsableState.Never)] attribute to UseWorkloadSets to hide it from completion? I love this little hack: https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.editorbrowsableattribute?view=net-9.0
There was a problem hiding this comment.
Never heard of that, but that should be doable!
There was a problem hiding this comment.
Did you get a chance to try this out?
There was a problem hiding this comment.
Ah, forgot to. Now I'm split because I think it's a positive change, but I'm pretty reluctant to retrigger tests when they're passing for a once...
There was a problem hiding this comment.
Apparently this doesn't work anyway, unfortunately. It isn't available on some of the frameworks where it's needed:
There was a problem hiding this comment.
Wait, I'm probably stupid? That seems like an editor bug; it should be available on net10.0
There was a problem hiding this comment.
After a bit more poking around, I suspect this just doesn't work properly in my IDE, so I'll push the change. If it fails, I can revert it.
| @@ -37,7 +37,8 @@ public void GivenNoWorkloadsAreInstalledListIsEmpty() | |||
| command.Execute(); | |||
|
|
|||
| // Expected number of lines for table headers | |||
| _reporter.Lines.Count.Should().Be(6); | |||
| // Expecting a workload set adds two lines | |||
There was a problem hiding this comment.
This comment was helpful, thanks :)
There was a problem hiding this comment.
I like this approach for turning it on; this was well designed! It looks good to me, though I think we should announce when we make workload sets on by default. I dont think we've been very public about workload sets or probably most people dont know what they are.
Is there an announcement page for this or a breaking change doc we're going to have?
Forgind
added
breaking-change
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
Starting this as a draft because I can almost guarantee that some test will fail.