Closed slashmili closed 8 years ago
@tonini I'm done! Can you review the code and give your feedback?
Hi @slashmili
First, thanks a lot for all your work you invested into this, it's really great and I like the idea. :)
I'm still no reviewed the whole thing but the first thing I saw and I'm not sure about it the device
argument which is now needed everywhere.
What do you think about using ets
like elixir does it for storing the iex
server? Have a look here https://github.com/elixir-lang/elixir/search?utf8=%E2%9C%93&q=autocomplete_server
/cc @lexmag @gausby @msaraiva It would be great if you guys also could review the PR too, you guys using the alchemist-server too and @msaraiva has great experience with using it and extend it as well.
Cheers
And now let me eat my chocolate bunny please ;-)
Thanks for you feedback. I'm not quite sure about your suggestion. In network connection case, there might be multiple clients using the same server on the same time and I'm not sure if registering in a process in ets
would help. Because there would be multiple process and each one answers to different client.
I can make change for the APIs so they return the result back to Alchemist.Server.IO
and Alchemist.Server.Socket
. Each of these servers know how to put the data back to their own clients. What do you think about this idea?
@slashmili It would be great if you could squash all commits to one with a proper commit message. https://github.com/tonini/alchemist.el/blob/master/CONTRIBUTING.md#commit-messages
Sure will do. Sorry for polluting the history :)
haha don't worry :) I just stumbled over it and thought it might be a good idea given that this PR brings new cool things.
@tonini I'll take a look at it. @slashmili I just love this feature. As you can see, it was something already planned for atom-elixir as well: https://github.com/msaraiva/atom-elixir/issues/16
@msaraiva great! We are almost there.
Just a bit background about communication to the server, I wanted to use unix socket but apparently gen_tcp doesn't support AF_UNIX.
Instead Alchemist.Server.Socket
is listening to first available port and print it out to STDOUT
and the client can read the connection details from STDOUT
and connect to the server.
@tonini for the case that you mentioned the device
needs to be passed in all the function, it's a valid point. I made changes in PING command, It covers your concern but it means that each commands should return the result as string instead of printing it out. If you are fine with that I'll refactor the rest of the commands to work like that.
As your suggestion for squashing the commits, I'll do it when we are close to the merge.
@slashmili I think that would be a good way if you're also ok with it.
Ok, I'll make the change then.
@tonini I made the changes
Squashed the changes in a commit and created PR #9
By this change Alchemist.Server can listen to a port and reply the command through socket connection:
Starting the server
Then the client connects and talk to the server like this:
This changes allows us to introduce vim integration, if your interested to see how it would work, take look at alchemist.vim