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

Actionabilize errors from incorrect settings files #1263

Conversation

travisclagrone
Copy link
Contributor

@travisclagrone travisclagrone commented Jun 18, 2019

Fixes #1053 "Errors from incorrect settings files are not actionable".

@travisclagrone travisclagrone changed the title Revert "Annotatively research current state of exception handling surrounding settings file load invocation" Actionabilize errors from incorrect settings files Jun 18, 2019
@travisclagrone
Copy link
Contributor Author

The first commit adds only research comments, and the last commit reverts them. However, I have chosen to leave the commits in for documentary purposes in the history. In particular, I imagine that further work either refactoring or extending this code region might benefit from them.

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

First of all thanks for making the effort and improvement. I agree that throwing a terminating error here is the right decision. One minor comment but hold off from addressing it at the moment, I will add more reviewers so that they can decide as well. But otherwise, it looks good to me. In the future we might enhance it to emit DiagnosticRecords of type ParserError in lower levels so that people get highlighting in VS Code but your change to improve the command line experience is good as a first improvement.
The red Ubuntu build can be ignored for the moment, this is a problem with the recent image update in AppVeyor that is also affecting the main development branch.
With the recently merged PR #1250, which improves the exceptions that are reported, this gives us now nice messages like this @JamesWTruher :-)

Invoke-ScriptAnalyzer -ScriptDefinition gci  -Settings @{foo='bar'}
Invoke-ScriptAnalyzer : foo is not a valid key in the settings hashtable. Valid keys are ExcludeRules, IncludeRules and Severity.
At line:1 char:2
+  Invoke-ScriptAnalyzer -ScriptDefinition gci  -Settings @{foo='bar'}
+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidData: (System.Collections.Hashtable:Hashtable) [Invoke-ScriptAnalyzer], InvalidDataException
+ FullyQualifiedErrorId : InvalidSettingsData,Microsoft.Windows.PowerShell.ScriptAnalyzer.Commands.InvokeScriptAnalyzerCommand

Engine/Commands/InvokeScriptAnalyzerCommand.cs Outdated Show resolved Hide resolved
@bergmeister bergmeister changed the base branch from development to master June 18, 2019 21:11
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

I think @JamesWTruher was working on something in this space as well.

Ideally this should actually throw an AggregateException and tell you all of the configuration problems (that's the general pattern for a parser, so you can fix as much as possible).

But I think this is a step in the right direction.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 19, 2019

@bergmeister agreed re integration for other contexts; ideally rather than a parse error the error should be something PSES can catch and then display as a notification. We should discuss what the best design is there, especially in terms of revising the hosting model (@JamesWTruher)

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me, just one minor cleanup nit

@@ -8,6 +8,7 @@
using System.Collections.ObjectModel;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: With the last commit, this using statement is not needed any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using System.IO; does appear to be required for pre-existing unqualified references to the type System.IO.InvalidDataException.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not any more after the last commit that removed this type. I pushed a last commit to fix this and do some minor cleanup and make the format consistent to the solution and merge then after this.

…nvoke-ScriptAnalyzer -Settings' results in an exception
…ScriptAnalyzer -Settings` results in an exception
…rounding settings file load invocation"

This reverts commit 3bfd45b.
…processing the argument of `Invoke-ScriptAnalyzer -Settings` results in an exception
@travisclagrone travisclagrone force-pushed the actionabilize-settings-file-errors branch from 1fc5c77 to 88ab1ec Compare June 23, 2019 00:35
This reverts commit 88ab1ec.

`using System.IO` is required in Engine\Settings.cs after all. The main
use is for pre-existing unqualified references to the type
InvalidDataException.
…name and make error record arguments consistent with usage in other parts of the solution.
@bergmeister bergmeister merged commit b407e8e into PowerShell:master Jun 29, 2019
@travisclagrone travisclagrone deleted the actionabilize-settings-file-errors branch June 29, 2019 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors from incorrect settings files are not actionable
3 participants