| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
You are on the right track. I suggest to continue focusing on the code of the rule itself. Don't worry about the rest like docs or some other test failures at the moment, they will be easy to fix at the end. |
Sorry, something went wrong.
…en rule development
|
Christoph Bergmeister (@bergmeister) I've hit a milestone. At this point the code seems to be working as intended. I have added one minor feature which may need to be in a separate PR: I've added the option to use Test-ScriptAnalyzer with a -RuleToTest parameter, so I could easily make adjustments, run .\build.ps1, and run Test-ScriptAnalyzer -RuleToTest 'avoiduni' -ShowAll to check if the change made it worse or better :-). Can we have a little brainstorm on where this rule may be applied to, other than just for Foreach-Object -Parallel? I'm thinking of maybe Invoke-Command, but other than that? [Edit: a list of commands where this could be applied to can be found here] Would you like to start a review round at this point, or better after we have a clear picture of how to move forward? |
Sorry, something went wrong.
|
The failure in Ubuntu 1804 build is the same Authenticode glitch we saw earlier. |
Sorry, something went wrong.
|
// brainstorm Other situations where this rule can be applied:
|
Sorry, something went wrong.
|
Michael Klement (@mklement0) thanks for creating the powershell-docs PR for documenting this. That gave me the quick overview I needed. If you have any suggestions, I'd love to hear them, thanks. |
Sorry, something went wrong.
|
Thanks, Jos Koelewijn (@Jawz84), a great rule to add, and it would be great if it covered the other $using: contexts too (as you outline above). One pending PR to watch out for: PowerShell/PowerShell#11829, which is trying to eliminate overzealous $using: checks for nested script blocks, as described in PowerShell/PowerShell#11817 As for the terminology in the rule documentation: about_Remote_Variables refers to $using: as the Using scope modifier - it's not a directive. |
Sorry, something went wrong.
|
Thanks for pointing these out Michael Klement (@mklement0) ! |
Sorry, something went wrong.
|
The rule is working now. I am not completely satisfied with what the implementation looks like, and I'm likely to do some cleanup and refactor. If Michael Klement (@mklement0) or Christoph Bergmeister (@bergmeister) have insights how to best convert this to cleaner code, I'm all ears. Second question: when starting out, I named the rule AvoidUnInitializedVarsInNewRunspaces Is that the best, most descriptive name for this rule? |
Sorry, something went wrong.
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
…el_rule' into 1410_foreach_parallel_rule resolve merge conflict
to avoid closure allocation
|
Rob Holt (@rjmholt) I've been thinking about what to optimize, and especially: how to measure if it has worked. When I run tests, most are around 15ms, but some stand out and take longer: |
Sorry, something went wrong.
|
Rob Holt (@rjmholt) did you have a chance to look at this yet? I'm starting to fear we might be accidentally waiting for each other |
Sorry, something went wrong.
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
refactoring FindVarsInAssignmentAsts to return a dictionary in progress
|
Really enjoying your feedback Rob Holt (@rjmholt)! I am learning a lot. |
Sorry, something went wrong.
refactoredFindVarsInAssignmentAsts to return a dictionary
…el_rule' into 1410_foreach_parallel_rule fix merge conflict
|
Rob Holt (@rjmholt) back over to you, all good here. |
Sorry, something went wrong.
|
Christoph Bergmeister (@bergmeister), Rob Holt (@rjmholt) thanks for all the reviews. Is there anything more I can do here? I am uncertain what the next step is. Is this blocked on review from James Truher (@JamesWTruher)? |
Sorry, something went wrong.
|
I think having at least one reviewer from Microsoft is good enough, this is our unwritten rule, so I'll therefore merge. Congrats and thanks for your hard work |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Fixes #1410
PR Checklist