GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — UseConsistentWhitespace - Create option to ignore assignment operator inside hash table by daviesj · Pull Request #1566 · PowerShell/PSScriptAnalyzer Conversation
|
| public override AstVisitAction VisitScriptBlock(ScriptBlockAst scriptBlockAst) | ||
| { | ||
| return Visit(scriptBlockAst); | ||
| } | ||
|
|
||
| public override AstVisitAction VisitNamedBlock(NamedBlockAst namedBlockAst) | ||
| { | ||
| return Visit(namedBlockAst); | ||
| } | ||
|
|
||
| public override AstVisitAction VisitAssignmentStatement(AssignmentStatementAst assignmentStatementAst) | ||
| { | ||
| return Visit(assignmentStatementAst); | ||
| } | ||
|
|
||
| public override AstVisitAction VisitCommandExpression(CommandExpressionAst commandExpressionAst) | ||
| { | ||
| return Visit(commandExpressionAst); | ||
| } | ||
|
|
||
| public override AstVisitAction VisitHashtable(HashtableAst hashtableAst) | ||
| { | ||
| return Visit(hashtableAst); | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Given its general name, this visitor should probably have overrides for all AST types so that it will reliably work for all position visits. The alternative is to implement this class as a private nested class for the relevant rule.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Fully intend to implement all overrides. Just didn't want to produce that much code before I knew I was barking up the right tree. It flummoxed me a bit when I first decided to work on this that there is no good way to search a PowerShell AST based on script position without writing 200+ lines of code. That is why I had tried the simpler solution first (that I agree with you was not ideal by a long shot).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Well there is another way, which is to use FindAll() instead:
IEnumerable<Ast> astsContainingToken = scriptAst.FindAll(foundAst => ContainsTokenExtent(token, foundAst), /* search through nested ASTs -- I can't remember the correct parameter name */ true); // Find the containing AST with the smallest extent Ast smallestContainingAst = astsContainingToken.First(); foreach (Ast containingAst in astsContainingToken) { if (HasSmallerExtent(smallestContainingAst, containingAst)) { smallestContainingAst = containingAst; } }
Naturally you'd need to write the methods I've used there, but they basically contain logic you've already written
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I did try something like that but ran into a problem because apparently Ast's can contain another of exactly the same size so there isn't always a smallest. At that point I figured as many issues as I have run into so far trying to use FindAll(), it would be better just to go the visitor route.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Not sure if I have the preprocessor version directives just right in FindAstPositionVisitor.cs. I was puzzled that there isn't a |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
54351c0 to
94329b0
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your efforts. Only a minor suggestion from my side but I am happy with it from a code and test perspective, also tested it locally. Having the IgnoreAssignmentOperatorInsideHashTable setting will allow us to introduce this new functionality first as 'off by default, i.e. one has to opt-in' in VS-Code and once proven robust, I am even thinking of removing that setting in future versions and enabling it by default or do you think there are still use cases to keep the setting long term Joel Davies (@daviesj) ?
Rob Holt (@rjmholt) Can you re-review please?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| /// <summary> | ||
| /// Traverses the AST based on offests to find the leaf node which contains the provided <see cref="IScriptPosition"/>. | ||
| /// This method implements the entire functionality of this visitor. All <see cref="AstVisitor2"/> methods are overridden to simply invoke this one. | ||
| /// </summary> | ||
| /// <param name="ast">Current AST node to process.</param> | ||
| /// <returns>An <see cref="AstVisitAction"/> indicating whether to visit children of the current node.</returns> | ||
| private AstVisitAction Visit(Ast ast) | ||
| { | ||
| if (ast.Extent.StartOffset > searchPosition.Offset || ast.Extent.EndOffset <= searchPosition.Offset) | ||
| { | ||
| return AstVisitAction.SkipChildren; | ||
| } | ||
| AstPosition = ast; | ||
| return AstVisitAction.Continue; | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
As a private method, I would put this one below all the others
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Unable to use both AlignAssignmentStatement and UseConsistentWhitespace rules
There was an error while loading. Please reload this page.