Add default installer precedence if not defined by user#6123
Add default installer precedence if not defined by user#6123Trenly wants to merge 5 commits intomicrosoft:masterfrom
Conversation
|
@jsoref - I'm not sure I understand why this error is coming up, when it didn't come up on PRs from a few days ago and nothing has changed in the workflow file. Any advice? |
Unfortunately the only choices are updating to the prerelease version (pinned by sha) or adding a guard that skips check-spelling for PRs where the base repo doesn't match the head repo (the check only applies in that case). |
| requirement = Settings::User().Get<Settings::Setting::InstallerTypeRequirement>(); | ||
|
|
||
| // Apply default precedence order when the user has not configured any installer type preferences or requirements. | ||
| if (preference.empty() && requirement.empty()) |
There was a problem hiding this comment.
Feels like this should be set any time preferences are not provided.
Of if requirements are provided but preferences are not, then we set preferences to requirements by default.
There was a problem hiding this comment.
I thought it could be confusing if the requirements were copied into the preferences, unless that would be useful for dependency selection?
There was a problem hiding this comment.
Alternatively, we implicitly "copy" the preferences by using the requirements in IsFirstBetter when the preferences are empty. That is probably cleaner than copying things in memory.
If preferences are empty, then you get manifest ordering (when other fields are equal of course) among the types in the requirements. I don't think we want a case where the manifest has two types and you get one or the other based on whether you have requirements set to include both (unless it is using requirement ordering).
I feel like implicitly using the requirement ordering is the better usability option, and it feels natural to me to list my requirements in the order that I would prefer.
There was a problem hiding this comment.
AH, I see now - Currently the requirements are just an applicability filter that work like "Any of these are valid" but the ordering is non-consequential. By copying the requirements to the preferences (or using them in IsFirstBetter) it would allow for the ordering that the preferences give.
That makes sense to me!!
There was a problem hiding this comment.
I think the order I put my requirements or my preferences in should be treated as an ordered list.
4 participants
This PR adds a default order of precedence for installer types if the user has not specified one. This ensures that ordering in a manifest is less important. While this has the potential to cause some upset with publishers who would prefer a different default order, the linked issue can remain open for tracking those requests.
Future work could involve merging the default preferences with user preferences so that any installer types the user has not specified as their preference still adhere to this default ordering, but I presumed that to be out of scope for this feature
Microsoft Reviewers: Open in CodeFlow