Skip to content
This repository was archived by the owner on Sep 21, 2019. It is now read-only.

Conversation

@ches
Copy link
Contributor

@ches ches commented Aug 15, 2016

This is an incremental refactoring in response to #330 and the discussion that spawned it: #328 (comment)

This does not attempt to solve some broader design issues discussed in #330, it’s just a small localized change to separate the logic for the very different types of requests that StringResponse can be returned for, so it's more obvious that this handler has these different roles. This is in part to help factor out the direct usage of the editor to finish #328. I've made these private functions for now, they're not really handler functions for discrete ENSIME server response message types like the rest of the formal ProtocolHandler interface. They will might eventually fit logically somewhere else.

As I've mentioned elsewhere, I don't really understand the purpose of the :EnDocUri command, it used to print the documentation URL in Vim's message area—which I don't see as useful—but even that is broken on current master (the command actually causes an exception). My guess is that the intent was that it could be useful as a function instead of command, allowing users to do something custom with the URL as a function result. As-is I don't yet see how to cleanly change it to work that way, it would have to be made synchronous. So with this change, I have un-documented the command but left some implementation in place with a TODO (and it no longer causes an exception, just basically a no-op). I could just go all the way and completely remove the command and that handling code right now, WDYT?

StringResponse is served in response to several quite different
requests and the existing handler code made that very unclear. This
does not yet attempt to solve some broader design issues discussed in
#330, it’s just a small refactoring to make the intent clearer in this
particular scope.
@ktonga
Copy link
Contributor

ktonga commented Aug 15, 2016

LGTM

@ktonga
Copy link
Contributor

ktonga commented Aug 15, 2016

Let's merge it so you can finish with #328

@ches
Copy link
Contributor Author

ches commented Aug 15, 2016

Thanks, I guess I need to LGTM myself since I don't know if anyone else is actively reviewing these days 😄

About to step AFK, should be able to wrap up #328 tomorrow.

@ches
Copy link
Contributor Author

ches commented Aug 15, 2016

LGTM

@ktonga
Copy link
Contributor

ktonga commented Aug 15, 2016

I thought it was configured to need just one LGTM

@ktonga
Copy link
Contributor

ktonga commented Aug 15, 2016

The check seems to be not working

@ches
Copy link
Contributor Author

ches commented Aug 15, 2016

I thought so too… I'm not going to like this thing if it's broken…

@ktonga
Copy link
Contributor

ktonga commented Aug 15, 2016

Re: Doc URI:

I think, if it is possible, it would be nice to add an optional arg to :EnDoc which should act as action. Missing arg means browse (open in browser) and any provided value should be the name of a function that takes a string (the resultant URI) as parameter and performs whatever action is desired. :EnDocUri should be removed.

Open doc as usual:
:EnDoc

Perform custom action with URI:
:EnDoc ShowMessage

Somewhere in the vimrc:

function! ShowMessage(msg)
    echom a:msg
endfunction

@ktonga
Copy link
Contributor

ktonga commented Aug 15, 2016

LGTM

@ktonga ktonga merged commit f5418db into ensime:master Aug 15, 2016
@ches ches deleted the handle_string_response-clarity branch August 16, 2016 14:08
@ches ches mentioned this pull request Aug 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants