-
Notifications
You must be signed in to change notification settings - Fork 369
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add UseUsingScopeModifierInNewRunspaces rule #1419
Add UseUsingScopeModifierInNewRunspaces rule #1419
Conversation
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. |
…en rule development
@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 Can we have a little brainstorm on where this rule may be applied to, other than just for Would you like to start a review round at this point, or better after we have a clear picture of how to move forward? |
The failure in Ubuntu 1804 build is the same Authenticode glitch we saw earlier. |
// brainstorm Other situations where this rule can be applied:
|
@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. |
Thanks, @Jawz84, a great rule to add, and it would be great if it covered the other One pending PR to watch out for: PowerShell/PowerShell#11829, which is trying to eliminate overzealous As for the terminology in the rule documentation: |
Thanks for pointing these out @mklement0 ! |
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 @mklement0 or @bergmeister have insights how to best convert this to cleaner code, I'm all ears. Second question: when starting out, I named the rule |
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
@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: |
@rjmholt did you have a chance to look at this yet? I'm starting to fear we might be accidentally waiting for each other |
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 @rjmholt! I am learning a lot. |
refactoredFindVarsInAssignmentAsts to return a dictionary
…el_rule' into 1410_foreach_parallel_rule fix merge conflict
@rjmholt back over to you, all good here. |
@bergmeister, @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 @JamesWTruher? |
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 |
PR Summary
Fixes #1410
PR Checklist
.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.