Add separate function for VS Code arguments#4599
Merged
code-asher merged 2 commits intocoder:mainfrom Dec 10, 2021
Merged
Conversation
The problem before was that the pop() caused the open in existing instance functionality to break because the arguments no longer contained the file. We could simply remove the pop() but since `workspace` and `folder` are not CLI arguments I think it makes sense to handle them in a separate function which can be called at the point where they are needed. This also lets us de-duplicate some logic since we create these arguments in two spots and lets us skip this logic when we do not need it. The pop() is still avoided because manipulating a passed-in object in-place seems like a risky move. If we really need to do this we should copy the positional argument array instead.
026caf4 to
009b55c
Compare
Codecov Report
@@ Coverage Diff @@
## main #4599 +/- ##
==========================================
+ Coverage 65.61% 66.14% +0.52%
==========================================
Files 30 30
Lines 1652 1654 +2
Branches 330 331 +1
==========================================
+ Hits 1084 1094 +10
+ Misses 481 475 -6
+ Partials 87 85 -2
Continue to review full report at Codecov.
|
jsjoeio
approved these changes
Dec 10, 2021
Contributor
jsjoeio
left a comment
There was a problem hiding this comment.
refactoring, cleanup and tests - what a lovely friday gift! nice work 😎
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The problem before was that the pop() caused the open in existing
instance functionality to break because the arguments no longer
contained the workspace or directory (files are left untouched).
We could simply remove the pop() but since
workspaceandfolderarenot CLI arguments I think it makes sense to handle them in a separate
function which can be called at the point where they are needed. This
also lets us de-duplicate some logic since we create these arguments in
two spots and lets us skip this logic when we do not need it.
The pop() is still avoided because manipulating a passed-in object
in-place seems like a risky move. If we really need to do this we
should copy the positional argument array instead.