Skip to content
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

Remove duplicated message when no upgrades #1986

Merged
merged 3 commits into from
Mar 3, 2022

Conversation

felipecrs
Copy link
Contributor

@felipecrs felipecrs commented Mar 1, 2022


This is kind of a follow up of #1982, continuing the cleanup. But this is a bit different because it also changes one message.

This is how it was before:

winget upgrade --all
No installed package found matching input criteria.
10 packages have version numbers that cannot be determined. Use "--include-unknown" to see all results.
No applicable update found.

This is how it is now:

winget upgrade --all
No applicable update found.
10 packages have version numbers that cannot be determined. Use "--include-unknown" to see all results.

Please let me know what you think.

Microsoft Reviewers: Open in CodeFlow

@felipecrs felipecrs requested a review from a team as a code owner March 1, 2022 03:16
@felipecrs
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1986 in repo microsoft/winget-cli

@@ -759,7 +759,12 @@ namespace AppInstaller::CLI::Workflow

if (table.IsEmpty())
{
context.Reporter.Info() << Resource::String::NoInstalledPackageFound << std::endl;
if (m_onlyShowUpgrades) {
context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl;
Copy link
Contributor

@yao-msft yao-msft Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want to differentiate "app not installed" vs "update not applicable". This change seems ok with winget upgrade --all, but we want clearer message for winget upgrade <specific package>

If we want to avoid showing "app not installed" for winget upgrade --all, it would be done as not triggering this reporting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I restored the original message, but at least removed the duplication. What do you think?

@yao-msft
Copy link
Contributor

yao-msft commented Mar 3, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft yao-msft merged commit 0a6cb30 into microsoft:master Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants