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

Add Friendlier Messages for Error Codes #3201

Merged
merged 6 commits into from
May 10, 2023
Merged

Conversation

Trenly
Copy link
Contributor

@Trenly Trenly commented May 1, 2023

Not all the error codes had a friendly message associated. This PR adds them.


Microsoft Reviewers: Open in CodeFlow

@Trenly Trenly requested a review from a team as a code owner May 1, 2023 20:36
@github-actions

This comment has been minimized.

@Trenly Trenly mentioned this pull request May 1, 2023
2 tasks
@JohnMcPMS
Copy link
Member

While this change isn't inherently bad, the correct solution that just hasn't been done is to have this function return an AppInstaller::StringResource::String, then have a function to get the internal, non-localized string message from that string name. The CLI paths would prefer to get the localized resource value, falling back to the internal path if one isn't present (and hopefully never needing to do that).

Not expecting you to do any of this, just dumping my "we should do this one day" thoughts out there...

@Trenly
Copy link
Contributor Author

Trenly commented May 1, 2023

While this change isn't inherently bad, the correct solution that just hasn't been done is to have this function return an AppInstaller::StringResource::String, then have a function to get the internal, non-localized string message from that string name. The CLI paths would prefer to get the localized resource value, falling back to the internal path if one isn't present (and hopefully never needing to do that).

Not expecting you to do any of this, just dumping my "we should do this one day" thoughts out there...

I completely agree with you; Definitely something to add to the "To-Do" list. If I have time this weekend I can look into that (maybe)

@yao-msft
Copy link
Contributor

yao-msft commented May 5, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

There was an error handling pipeline event dbf1b29e-4f42-4b0d-a9ce-115ec04168e1.

src/AppInstallerSharedLib/Errors.cpp Outdated Show resolved Hide resolved
src/AppInstallerSharedLib/Errors.cpp Outdated Show resolved Hide resolved
src/AppInstallerSharedLib/Errors.cpp Outdated Show resolved Hide resolved
Co-authored-by: yao-msft <50888816+yao-msft@users.noreply.github.com>
@yao-msft
Copy link
Contributor

yao-msft commented May 9, 2023

Nit: can you remove the extra line, not sure if it's github's apply suggestion issue.

@yao-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yao-msft yao-msft merged commit b83529b into microsoft:master May 10, 2023
@Trenly Trenly deleted the ErrorCodes branch May 11, 2023 14:22
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.

4 participants