X Tutup
The Wayback Machine - https://web.archive.org/web/20200918095210/https://github.com/PowerShell/PSScriptAnalyzer/pull/1263
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

@travis-c-lagrone
Copy link
Contributor

travis-c-lagrone commented Jun 18, 2019

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

@travis-c-lagrone travis-c-lagrone 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
@travis-c-lagrone
Copy link
Contributor Author

travis-c-lagrone commented Jun 18, 2019

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 left a comment

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 requested review from rjmholt and JamesWTruher Jun 18, 2019
@bergmeister bergmeister changed the base branch from development to master Jun 18, 2019
Copy link
Member

rjmholt left a comment

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
Member

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 left a comment

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;

This comment has been minimized.

@bergmeister

bergmeister Jun 22, 2019 Collaborator

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

This comment has been minimized.

@travis-c-lagrone

travis-c-lagrone Jun 23, 2019 Author Contributor

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

This comment has been minimized.

@bergmeister

bergmeister Jun 29, 2019 Collaborator

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
@travis-c-lagrone travis-c-lagrone force-pushed the travis-c-lagrone:actionabilize-settings-file-errors branch from 1fc5c77 to 88ab1ec Jun 23, 2019
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
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
license/cla All CLA requirements met.
@travis-c-lagrone travis-c-lagrone deleted the travis-c-lagrone:actionabilize-settings-file-errors branch Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.
X Tutup