-
Notifications
You must be signed in to change notification settings - Fork 521
Fix additional PowerShell warning (take two) #5100
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
Conversation
380302c to
d705b20
Compare
Since Show Session Menu always fully enumerates the iterator we needed to move the existence check into the generator. Fortunately the 'exists' method was already idempotent. I'd like this to be cleaner, but at least the tests now make sense (and required a stub fix).
d705b20 to
e0f2130
Compare
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.
If there's a better approach I cant think of it, since these are filesystem tasks the awaits are actually useful to free up the extension to do other things while the search is happening. Just one item that I see with exists() before I approve.
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.
LOTM (Looks Obtuse to Me)
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.
LOTM (Looks Obtuse to Me)
LOTM (Looks Obtuse to Me)
Unfortunately yes but I didn't think of a cleaner way even over the weekend. At least the tests are less obtuse!
Agreed, me either, was just a joke basically of LGTM because I can't figure out anything better :)
Since Show Session Menu always fully enumerates the iterator we needed to move the existence check into the generator. Fortunately the 'exists' method was already idempotent. I'd like this to be cleaner, but at least the tests now make sense (and required a stub fix).
This is a follow up to #5099 after further testing.
@JustinGrote can you think of a way to simplify this logic?