GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — UseConsistentWhitespace - Create option to ignore assignment operator inside hash table by daviesj · Pull Request #1566 · PowerShell/PSScriptAnalyzer

Conversation

@daviesj

@daviesj Joel Davies (daviesj) commented Aug 7, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

PR Summary

Fix #769 instead of creating a workaround. This creates an option on the UseConsistentWhitespace rule to ignore the assignment operator inside a multi-line hash table, making it compatible with the AlignAssignmentStatement rule. Also this enables the new option in the code formatting settings presets since they all enable the AlignAssignmentStatement rule. If I were a bit smarter, I would also automatically enable this option if AlignAssignmentStatement is enabled but I do not see how to do that.

PR Checklist`

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@daviesj
UseConsistentWhitespace - Create option to ignore assignment operator…
5dc3cd3
… inside hash table
@ghost

Deleted user (ghost) commented Aug 7, 2020 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rjmholt
Rob Holt (rjmholt) reviewed Aug 7, 2020
View reviewed changes
Comment thread Engine/Settings/CodeFormatting.psd1 Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/UseConsistentWhitespace.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/UseConsistentWhitespace.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/UseConsistentWhitespace.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Rules/UseConsistentWhitespace.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@daviesj
Undo whitespace changes to settings files
b9aa6e7
@daviesj
Joel Davies (daviesj) force-pushed the master branch from 4d1b67d to b9aa6e7 Compare August 10, 2020 17:55
@daviesj
Begin implementation of visitor to find token in AST
759c712
@daviesj Joel Davies (daviesj) changed the title UseConsistentWhitespace - Create option to ignore assignment operator inside hash table WIP: UseConsistentWhitespace - Create option to ignore assignment operator inside hash table Aug 11, 2020
rjmholt
Rob Holt (rjmholt) reviewed Aug 11, 2020
View reviewed changes
Comment thread Engine/TokenOperations.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Engine/TokenOperations.cs Outdated
Comment on lines +290 to +313
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);
}

@rjmholt Rob Holt (rjmholt) Aug 11, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@daviesj Joel Davies (daviesj) Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rjmholt Rob Holt (rjmholt) Aug 11, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@daviesj Joel Davies (daviesj) Aug 12, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@daviesj
Fleshed out FindAstPositionVistor and fixed test
7ef643b
@rjmholt
Rob Holt (rjmholt) marked this pull request as draft August 12, 2020 00:13
@daviesj

Joel Davies (daviesj) commented Aug 12, 2020

Copy link
Copy Markdown
Contributor Author

Not sure if I have the preprocessor version directives just right in FindAstPositionVisitor.cs. I was puzzled that there isn't a PSV5 constant and building for PowerShell 4 sets both PSV3 and PSV4.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Joel Davies (daviesj) added 2 commits August 12, 2020 10:52
@daviesj
Make visitor internal
047ae5e
@daviesj
correct spelling
94329b0
@daviesj
Joel Davies (daviesj) force-pushed the master branch from 54351c0 to 94329b0 Compare August 25, 2020 21:42
@daviesj Joel Davies (daviesj) changed the title WIP: UseConsistentWhitespace - Create option to ignore assignment operator inside hash table UseConsistentWhitespace - Create option to ignore assignment operator inside hash table Sep 16, 2020
@daviesj
Joel Davies (daviesj) marked this pull request as ready for review September 16, 2020 16:22
@daviesj
fix off-by-one error in visitor
cc9ebd7
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Oct 24, 2020
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Engine/FindAstPositionVisitor.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@daviesj
Simplify code in visitor
ff96591
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Oct 26, 2020
View reviewed changes
@daviesj Joel Davies (daviesj) mentioned this pull request Oct 27, 2020
WIP: Possible alternate implementation of UseConsistentWhitespace CheckOperator feature #1607
Draft
3 of 6 tasks
@rjmholt
Rob Holt (rjmholt) self-requested a review November 3, 2020 19:04
rjmholt
Rob Holt (rjmholt) reviewed Nov 10, 2020
View reviewed changes
Comment thread Engine/FindAstPositionVisitor.cs Outdated
Comment on lines +33 to +47
/// <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;
}

@rjmholt Rob Holt (rjmholt) Nov 10, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
rjmholt
Rob Holt (rjmholt) approved these changes Nov 10, 2020
View reviewed changes
@daviesj
Move private method to the bottom & small summary correction (leaf =>…
3594ed5
… leaf-most)
@JamesWTruher
James Truher (JamesWTruher) merged commit 5797a04 into PowerShell:master Jan 5, 2021
This was referenced Aug 25, 2021
Pin PSScriptAnalyzer to 1.20.0, Plaster to 1.1.3 and PSReadLine to 2.1.0 PowerShell/PowerShellEditorServices#1562
Merged
Update bundled PSScriptAnalyzer to v1.20.0 PowerShell/vscode-powershell#3524
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergmeister Christoph Bergmeister (bergmeister) bergmeister approved these changes
+1 more reviewer
@rjmholt Rob Holt (rjmholt) rjmholt approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unable to use both AlignAssignmentStatement and UseConsistentWhitespace rules

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@daviesj @rjmholt @bergmeister @JamesWTruher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.