Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(GH-#187) Add a stdio mode to the language server #188

Merged
merged 5 commits into from
Nov 12, 2017
Merged

(GH-#187) Add a stdio mode to the language server #188

merged 5 commits into from
Nov 12, 2017

Conversation

ananace
Copy link
Contributor

@ananace ananace commented Nov 6, 2017

I'm unsure on how to add a test for this feature, but if someone has a good suggestion for that then I'll happily write one.

@glennsarti
Copy link
Contributor

Thanks for the PR @Ace13 . I'll review it soon.

@glennsarti
Copy link
Contributor

@Ace13 Are you ok with me adding on some commits to this PR?

@glennsarti
Copy link
Contributor

Couple of things I see

  • Client detection is a bit off so the timeout doesn't work yet
  • I'll have a go at some basic tests for both stdio and tcp based servers
  • I want to add a failure if you specify --stdio and --debug=STDOUT as they're not compatible

@ananace
Copy link
Contributor Author

ananace commented Nov 10, 2017

I didn't touch any of that client detection, since there's technically a client through the entirety of the servers lifetime. Not sure if that's the right way to go about it though, or if actually tracking a client is more appropriate.

I also did add a small mode for specifying --debug=STDERR during my testing, but didn't want to commit it as part of this feature addition as it's completely irrelevant to the feature itself. But yeah, should probably have an error for debug and stdio mode at the same time.

And I don't particularly mind if you want to add some commits as well.

@glennsarti
Copy link
Contributor

Yeah...just trying to think, if the language server is started as a child process, and the parent dies, then the child is orphaned. Will the language server be able to detect that ... I'll see if I can set that up as testing scenario.

@glennsarti
Copy link
Contributor

glennsarti commented Nov 11, 2017 •
edited

Yup...the language server remains if the parent dies. At least on Windows :-(

Nope...forgot that the parent process is sometimes cmd.exe

@glennsarti
Copy link
Contributor

On second thoughts. I think I'm being too defensive. Will just write some tests and then merge it.

@glennsarti
Copy link
Contributor

Hrmm...so the TCP server code seems to be broken. Good news is, my tests were failing because of it! I'll keep working on it. Thanks @Ace13

@glennsarti
(maint) Fix language server logging to a file
734531f 
Previously the case compare was looking for a non zero result which meant even
if a debug file was specified, it still used STDOUT.  This commit now correctly
identifies when a user enters stdout as the debug destination.
@glennsarti
(puppetlabsGH-187) Add integration tests for STDIO and TCP Language S…
090c83b 
…ervers

This commit adds smoke integration tests for both the TCP and STDIO variants of
the Puppet Language Server.
@glennsarti
(puppetlabsGH-187) Partial Revert 3690ca2
8517ba8 
Previously commit 3690ca2 the Socket connection object was modified for the
STDIO server.  However this was causing issues and was not required.  This
commit partially reverts the changes in 3690ca2 to the TCP Server and
JSON Handler to the parent commit.
@glennsarti
(puppetlabsGH-187) Suppress warnings when loading gems
2de87ea 
Previously older ruby versions with older Puppet gems would emit a warning about
Fiddler and DL on STDOUT.  This would confuse a STDIO Client as it pollutes
STDOUT with non JSON compliant text.  This commit suppresses verbose error
messages during the gem require statements.
glennsarti
glennsarti approved these changes Nov 12, 2017
View reviewed changes
Copy link
Contributor

@glennsarti glennsarti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When merged this will create a GH issue to add this to the docs and to refactor the TCP Socket classes to be more generic.

@glennsarti
Copy link
Contributor

@Ace13 You ok with my additional commits?

@ananace
Copy link
Contributor Author

ananace commented Nov 12, 2017

No issues with them, looks fine to me.

@glennsarti glennsarti merged commit 55cc029 into puppetlabs:master Nov 12, 2017
This was referenced Nov 13, 2017
Closed
Closed
Closed
@glennsarti glennsarti added this to the 0.8.0 milestone Nov 16, 2017
@glennsarti glennsarti mentioned this pull request Apr 30, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glennsarti glennsarti glennsarti approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ananace @glennsarti @jordanbreen28