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

Plugins: propagate plugin exception #2838

Merged
merged 4 commits into from
May 3, 2019
Merged

Conversation

dtivel
Copy link
Contributor

@dtivel dtivel commented May 2, 2019

Resolve NuGet/Home#8057.

Ideally, NuGet.Common.ILogger would have a method that would take an exception and log the callstack depending on the logging verbosity level:

    void LogException(Exception exception);

However, that change is a large, breaking change and out of scope for fixing NuGet/Home#8057.

As it currently stands, it's left to the caller of an ILogger implementation to determine whether or not to log a callstack based on the logging verbosity level, but the logging verbosity level is unavailable to the caller.

The simple solution now is to log the callstack with ILogger.LogDebug(…) and leave it to the logger to decide whether or not to log debug events.

NuGet.sln Show resolved Hide resolved
@@ -87,9 +87,9 @@ Invoke-BuildStep 'Cleaning package cache' {
-skip:(-not $CI) `
-ev +BuildErrors

Invoke-BuildStep 'Running /t:RestoreVS15' {
Invoke-BuildStep 'Running /t:RestoreVS' {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes were overlooked by #2832.

Copy link
Member

Choose a reason for hiding this comment

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

My bad, we don't use these scripts in our build at all, so I didn't even think about them.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Looks great.
👏

@@ -87,9 +87,9 @@ Invoke-BuildStep 'Cleaning package cache' {
-skip:(-not $CI) `
-ev +BuildErrors

Invoke-BuildStep 'Running /t:RestoreVS15' {
Invoke-BuildStep 'Running /t:RestoreVS' {
Copy link
Member

Choose a reason for hiding this comment

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

My bad, we don't use these scripts in our build at all, so I didn't even think about them.

@dtivel dtivel force-pushed the dev-dtivel-plugin-exception-details branch from 5e57944 to 832e288 Compare May 3, 2019 00:04
@dtivel dtivel merged commit 0690359 into dev May 3, 2019
@dtivel dtivel deleted the dev-dtivel-plugin-exception-details branch May 3, 2019 03:55
dtivel added a commit that referenced this pull request May 3, 2019
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.

Plugins: exception details lost during plugin creation
2 participants