Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Actionabilize errors from incorrect settings files#1263

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@travisclagrone
Copy link
Contributor

@travisclagronetravisclagrone commentedJun 18, 2019
edited by bergmeister
Loading

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

bergmeister reacted with hooray emoji
@travisclagronetravisclagrone changed the titleRevert "Annotatively research current state of exception handling surrounding settings file load invocation"Actionabilize errors from incorrect settings filesJun 18, 2019
@travisclagrone
Copy link
ContributorAuthor

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

@bergmeisterbergmeister left a comment
edited
Loading

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 maindevelopment 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

rjmholt reacted with thumbs up emoji
@bergmeisterbergmeister changed the base branch fromdevelopment tomasterJune 18, 2019 21:11
Copy link
Contributor

@rjmholtrjmholt 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 anAggregateException and tell youall 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

@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)

bergmeister reacted with thumbs up emoji

Copy link
Collaborator

@bergmeisterbergmeister 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

usingSystem.Collections.ObjectModel;
usingSystem.Diagnostics.CodeAnalysis;
usingSystem.Globalization;
usingSystem.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
ContributorAuthor

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 typeSystem.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 commit3bfd45b.
…processing the argument of `Invoke-ScriptAnalyzer -Settings` results in an exception
@travisclagronetravisclagroneforce-pushed theactionabilize-settings-file-errors branch from1fc5c77 to88ab1ecCompareJune 23, 2019 00:35
This reverts commit88ab1ec.`using System.IO` is required in Engine\Settings.cs after all. The mainuse is for pre-existing unqualified references to the typeInvalidDataException.
…name and make error record arguments consistent with usage in other parts of the solution.
@bergmeisterbergmeister merged commitb407e8e intoPowerShell:masterJun 29, 2019
@travisclagronetravisclagrone deleted the actionabilize-settings-file-errors branchJune 29, 2019 17:37
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@bergmeisterbergmeisterbergmeister approved these changes

@JamesWTruherJamesWTruherAwaiting requested review from JamesWTruher

+1 more reviewer

@rjmholtrjmholtrjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Errors from incorrect settings files are not actionable

3 participants

@travisclagrone@rjmholt@bergmeister

[8]ページ先頭

©2009-2025 Movatter.jp