Implement `EnUsages` command which searches for symbol usages by ktonga · Pull Request #402 · ensime/ensime-vim

ktonga · 2017-08-06T15:19:38Z

No description provided.

ches · 2017-08-06T17:40:13Z

Woot, looking forward to trying this out!

Separately, still gotta figure out if whatever made the CI build stop working and then come back but not actually run anything can be remediated… #394 (comment)

jvican · 2017-08-07T06:29:09Z

@@ -94,6 +97,12 @@ def handle_type_inspect(self, call_id, payload):
    def show_type(self, call_id, payload):
        raise NotImplementedError()

+    def handle_source_positions(self, call_id, payload):
+        raise NotImplementedError()


Should we open an issue so that contributors fill this in at some point of the future?

Sorry, I'm not following. This is the ugly way for defining an abstract method in python, so it doesn't need to be filled here.

Anyway, after @ches finishes his big refactor I'll rework the handlers, I think resolving them for the server version using inheritance is a terrible design and I want to change it. Also they should depend on protocol version as opposed to on server version.

Oh I see, was not familiar with this. Thanks for elaborating.

jvican

This looks really nice @ktonga, looking forward to trying this thing out. It's a game changer for users of this plugin.

jvican · 2017-08-07T06:30:10Z

@ches Please, feel free to merge when you approve it 😄.

ktonga · 2017-08-07T06:50:05Z

Thanks @jvican it still has some small annoyances on the server side, nothing to do on vim's side tho, as soon as the results get better on the server we will see it on vim without effort. For instance I think it still doesn't work on methods.

ches

Issues reported inline that are show-stoppers on non-Neovim, but otherwise it works, woo!

ches · 2017-08-08T14:51:47Z

@@ -249,7 +249,7 @@ def send_at_position(self, what, useSelection, where="range"):
        self.log.debug('send_at_position: in')
        b, e = self.editor.selection_pos() if useSelection else self.editor.word_under_cursor_pos()
        self.log.debug('useSelection: {}, beg: {}, end: {}'.format(useSelection, b, e))
-        self.send_at_point_req(what, self.editor.path(), b[0], b[1], e[0], e[1], where)
+        self.send_at_range_req(what, self.editor.path(), b[0], b[1], e[0], e[1], where)


This send_at_position method was kind of unclear to begin with, but I think it gets even more so when we have send_at_position, send_at_point_req, and send_at_range_req, plus the latter has kept the where="range" parameter when it seems like that should invariantly be true now. And using "selection" is I guess different from "range"… 😩

I haven't looked much at how far it would reach, but can you see any way to refactor that makes all this clearer, or would it become a big change?

I know, right? the current state is far from ideal, but I tried to reduce the changes not related to the new feature to the minimum. I did take a look at the motivation for such a messy set of functions, and the thing is, that they are like this to hide some inconsistencies from the server's protocol, that arg is used to name the property in the message, some times is called 'range' and some times 'point' but it always carries the 'from' and 'to' that's why I renamed and called ...at_point... the proper one that actually sends the point.

I'd prefer to leave it like this by now, the sensible refactor to do here would be to introduce a model (with the case classe python equivalent) to represent the server messages instead of cryptic functions which create dics on the fly

I understand trying to keep changes minimal, but if this is now the only call site of the renamed send_at_range_req, could we drop its where parameter? I.e. branch in send_at_position if it was called with where='range' vs 'point'. Does it sometimes need to be called with where='point' to send a message with 'point': {'from': …, 'to': …}?

My concern with it is, previously you had one method send_at_point_req which could be called with either "point" or "range"… okay, that's a little bit weird to understand, but now you have two methods which seem to helpfully separate point vs. range yet actually you still sometimes might need to call the range one with a "point" argument! (Do you?)

I had started modeling protocol messages as namedtuples in my separate client project but the protocol was changing more than I was willing to keep up with… if that's slowed down maybe we could start doing that in ensime-vim in the short term (not in this PR of course).

Ok, so the thing is that at_range was only used once within send_at_position so I got rid of it and inlined the code in the later which is indeed called with 'point' a few times and in one case it uses the default value:

ensime_shared/client.py|357 col 14| self.send_at_position("Type", useSelection) ensime_shared/client.py|436 col 14| self.send_at_position("DocUri", False, "point") ensime_shared/client.py|451 col 14| self.send_at_position("DocUri", False, "point")

I've added some docs to the method to clarify the use of the parameters, should be a bit clearer now, still not ideal and I would like to improve it but any further refactor is out of the scope of this PR IMHO.

So now we only have two separate send_at... methods and they have clear different responsibilities, one sends from and to based on selection or word under cursor some times calling the field 'range' and other 'point', the other method simply sends the cursor position coordinates.

Nice, I think that narrowed responsibility of send_at_point is an improvement until we get more type-ful requests. Thanks!

ches · 2017-08-08T14:55:23Z

@@ -421,6 +429,15 @@ def doc_uri(self, args, range=None):
        self.log.debug('doc_uri: in')
        self.send_at_position("DocUri", False, "point")

+    def usages(self, args, range=None):


It seems we do this with basically every Vim function -> Python function dispatch and we've probably talked about it before in general, but args and range are unused arguments. Should we stop this madness?

Is it impossible currently because of the execute_with_client decorator's expectation of what it wraps?

Oh yeah, I think it was my complaint about this that prompted creation of #244. Don't recall if there's a blocker to stop proliferating the pattern, if not changing all existing ones yet.

Ditto unrelated changes.

I'd prefer to align to the current pattern and change them all later on a tech-debt PR

I don't consider things "unrelated changes" though when we're talking about newly-added code. If it's impossible to define this without the unused args and range params without changing other things to make it work, then okay that's fine. But if it is possible already (that's my question, I didn't try yet), what's the justification to blindly keep adding new code this way?

Turns out it is possible and really easy to do. Done!

ches · 2017-08-08T15:02:00Z

-    def write_quickfix_list(self, qflist):
-        self._vim.command('call setqflist({!s})'.format(qflist))
+    def write_quickfix_list(self, qflist, title):
+        self._vim.command("call setqflist({!s}, 'r', 'Ensime - {}')".format(qflist, title))


I believe this signature of setqflist is an incompatibility between Vim and Neovim. In Vim 8 the third argument is documented as being a dictionary and has this example:

:call setqflist([], 'r', {'title': 'My search'})

It looks like Vim 7 didn't support it at all and I think giving up Vim 7 support might be a bit premature now, Neovim difference aside.

Oh my, really good catch here, dropping Vim 7 support wasn't my intention at all 😄. I just naively followed nvim docs for the function trusting on them being compatible across vim versions/flavors.

Will rework this to make sure that ir runs everywhere (aka add a bunch of ifs 😞)

I unfortunately encountered exactly this API diversion once in the past 😞

Fixed. I've tested it on both, vim and nvim, and it works.

ches · 2017-08-08T15:07:19Z

+                                                preview,
+                                                "info")
+            qfList.append(item)
+        qfSorted = sorted(qfList, key=itemgetter('filename', 'lnum'))


It's coming from other instances elsewhere I know, but we're mixing style of naming conventions with qfList and qfSorted.

WDYM? there's only another instance where qfList is used, and introduced qfSorted to hold the sorted copy of qfList. How you reckon they should be called?

Oh I gotcha! you mean camelCase vs snake_case.

I can look around and turn all names into snakes, maybe there's even a linter setting to ensure that. But again, would prefer to do it on a separate PR since this was not introduced in this PR

Yep I meant the camelCase. Sure no need to change other instances outside this PR now, but this case is new right?

ches · 2017-08-08T15:33:57Z

@@ -73,6 +73,7 @@ command! -nargs=* -range EnInstall call ensime#com_en_install([<f-args>], '')
 command! -nargs=* -range EnNoTeardown call ensime#com_en_no_teardown([<f-args>], '')
 command! -nargs=* -range EnTypeCheck call ensime#com_en_type_check([<f-args>], '')
 command! -nargs=* -range EnType call ensime#com_en_type([<f-args>], '')
+command! -nargs=* -range EnUsages call ensime#com_en_type([<f-args>], '')


Should be com_en_usages 😊

Sorry, I might have killed a kitten with this ~~copy&paste~~ yank&put. Will fix it.

Fix copypasta

ches

LGTM, squash fixups and shipit!

Excited to use this.

ches · 2017-08-09T17:21:16Z

Oh, updating vimdoc would be a nice a finishing touch if you have the chance ☺️

ktonga · 2017-08-09T22:01:06Z

@ches oh you're right, totally forgot about the docs. Done.

Thank you very much for taking the time to review it guys.

ktonga requested review from ches and jvican August 6, 2017 15:19

ktonga mentioned this pull request Aug 6, 2017

Support for find usages and type hierarchy #400

Open

2 tasks

jvican reviewed Aug 7, 2017

View reviewed changes

jvican approved these changes Aug 7, 2017

View reviewed changes

ches suggested changes Aug 8, 2017

View reviewed changes

ktonga force-pushed the find-usages branch from 7c5e80a to f77faef Compare August 9, 2017 12:53

ches approved these changes Aug 9, 2017

View reviewed changes

Implement EnUsages command which searches for symbol usages

f9a15e0

ktonga force-pushed the find-usages branch from 27c5dad to f9a15e0 Compare August 9, 2017 21:56

ktonga merged commit ee95193 into ensime:master Aug 9, 2017

Conversation

ktonga commented Aug 6, 2017

Uh oh!

ches commented Aug 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

jvican commented Aug 7, 2017

Uh oh!

ktonga commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ches left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ches Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktonga Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ches left a comment

Choose a reason for hiding this comment

ktonga commented Aug 7, 2017 •

edited

Loading

ktonga Aug 8, 2017 •

edited

Loading

ktonga Aug 9, 2017 •

edited

Loading

ktonga Aug 8, 2017 •

edited

Loading

ktonga Aug 8, 2017 •

edited

Loading

ktonga Aug 8, 2017 •

edited

Loading

ches Aug 9, 2017 •

edited

Loading

ktonga Aug 8, 2017 •

edited

Loading