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.
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
Adds ability to use separate pipes for reading and writing #785
Adds ability to use separate pipes for reading and writing #785
Changes from 1 commit
9db4d7e
ea65162
8d20b53
c47e80b
4e747a1
0189966
569c223
93a391f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we used parameter sets then here we'd check if ($PSCmdlet.ParameterSetName -eq 'NamedPipeDuplex') and perhaps $LanguageServicePipe would be a mandatory parameter? Not sure about $DebugLanguageServicePipe, I suppose you could create one but not the other?
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.
This way of specifying two pipe names in a single string here works, but I'm not a huge fan of it.
Getting around it without a breaking change doesn't seem all that simple, but I think we should revisit this soon. Otherwise all our config options are going to end up named something abstract and needing a bunch of preprocessing to unspool, even though we have a typed, in-memory .NET API.
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.
Not suggesting this be changed in any way other than @tylerl0706's suggestion, but I don't think this should be the long term solution.
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.
Perhaps something to re-visit for v2?
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.
Are these lines here needed? Or would leaving them out have the same effect? Since they are PascalCased, I'm assuming they are script parameters, so the nulling could be done in the parameter definition if required.
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.
Not a script parameter. This variable is a local so it should be camelCased -
$languageServiceWritePipeName
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.
Ditto for
$DebugServiceWritePipeName
below.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.
Agree, missed this. Will make them as script parameters and move to a separate parameter set.
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.
It looks like this pattern is repeated 3 times. Would it be possible to make it a function?
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.
I'm more inclined to say that the character that separates the two file names should be the
DirectorySeparator
(System.IO.Path.DirectorySeparatorChar
) since;
could be a part of a valid file name. I don't thinkGetRandomFileName
would ever spit out a;
but at least if we use theDirectorySeparatorChar
we can be sure.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.
I spoke to @rjmholt about the use of
Endpoint
here. Historically, this was used to support TCP and NamedPipes but since we don't have TCP anymore, we could change thisEditorServiceTransportConfig
to have aPipe
andWritePipe
property (but named better) that way you don't have to do this string splitting.I think I'm ok if you don't want to do this in this PR - but it's pretty important so if you want to take it on I'd really appreciate it.
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.
Should the direction here be just
PipeDirection.In
?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.
Can use
var
here on the LHS instead ofList<Task>
.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.
I used
var
at first :) but having a look at other code here thought it is a coding style to use explicit types (including local vars). Can make itvar
if you'd like.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.
Ah, yeah the coding style in this project isn't great at the moment -- we've been trying to move it over gradually to being less redundant and verbose.
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.
Rethrow should never specify the exception - you'll lose stack info. Use
throw;
by itself.Ideally our logger methods would return false so you could use C# exception filters e.g.:
But we don't do that - yet. @rjmholt Any chance we could modify the log methods to return
false
for this reason?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.
Yeah, we could definitely modify the methods.
Or else write wrapper methods that explain the intended caller context:
Bit of a mouthful -- just some name that says "I return false for use with exception filters".