PsShouldProcess - ShouldContinue not included #1305
Conversation
There was a problem hiding this comment.
Thanks.
Code looks very clean and awesome that you added quite a few test cases :-)
I'd be very happy taking this PR.
After re-reading the docs on ShouldContinue, I have an idea for additional (optional) feature since the docs say:
Cmdlets using ShouldContinue should also offer a "bool Force" parameter which bypasses the calls to ShouldContinue and ShouldProcess.
https://docs.microsoft.com/en-us/dotnet/api/system.management.automation.cmdlet.shouldcontinue
If we could also check for the usage of the -Force pattern (not sure how difficult or possible it is), then that would be great. Again, this could be an optional, separate, follow up PR in addition to this awesome PR, would you be interested in doing that as well (no pressure)?
|
Thanks @bergmeister. I'd be happy to give it a go sometime; though my knowledge of parsers isn't great / this fix was easy to code since I could mostly steal from the existing efforts made for FYI: There is already a rule to ensure that the However, that rule can currently be fooled by adding the switch to the function's parameters, then neglecting to use it. I've created new issue: #1308 to track this. |
|
@rjmholt @JamesWTruher I would be happy to merge this change in, are you ok with it as well? |
| return functionDefinitionAst.Extent; | ||
| } | ||
|
|
||
| return (invokeMemberExpressionAstFound as InvokeMemberExpressionAst).Member.Extent; |
There was a problem hiding this comment.
| return (invokeMemberExpressionAstFound as InvokeMemberExpressionAst).Member.Extent; | |
| return ((InvokeMemberExpressionAst)invokeMemberExpressionAstFound).Member.Extent; | |
| /// </summary> | ||
| private static bool IsShouldContinueCall(Ast ast) | ||
| { | ||
| var invokeMemberExpressionAst = ast as InvokeMemberExpressionAst; |
There was a problem hiding this comment.
Might be nicer to do this:
return ast is InvokeMemberExpressionAst invokeMemberExpressionAst
&& invokeMemberExpressionAst.Member is StringConstantExpressionAst memberExprAst
&& string.Equals(memberExprAst.Value, "ShouldContinue", StringComparison.OrdinalIgnoreCase);| private bool callsShouldProcessDirectly(Vertex vertex) | ||
| { | ||
| return funcDigraph.GetNeighbors(vertex).Contains(shouldProcessVertex); | ||
| } | ||
| private bool callsShouldContinueDirectly(Vertex vertex) |
There was a problem hiding this comment.
| private bool callsShouldContinueDirectly(Vertex vertex) | |
| private bool callsShouldContinueDirectly(Vertex vertex) | |
|
@rjmholt Should we merge this PR as-is since @JohnLBevan hasn't provided any updates and did not tick the option for other people to allow push to his branch? We could then do a follow-up PR. |
|
Apologies, I missed your comments from Jan. I'm happy to make the suggested changes, though think I deleted the original branch a while back (i.e. the source of the pull request now shows as |
fixes #1304