Editor Object (#321) #328
Editor Object (#321) #328
Conversation
ensime_shared/ensime.py
Outdated
| self.ensime_cache = osp.join(config_dirname, ".ensime_cache") | ||
| self.log_dir = self.ensime_cache | ||
| if not osp.isdir(self.ensime_cache): | ||
| def __init__(self, editor, vim, launcher): # noqa: C901 |
There was a problem hiding this comment.
I can't quite jettison the vim attribute yet, the remaining usage is for disable_plugin which is too complicated to deal with in the course of these changes, I've dived into it a bit for #294 and will continue later.
| # Use current_file command because we cannot access self.vim | ||
| current_file_cmd = commands["current_file"] | ||
| current_file = self.vim.eval(current_file_cmd) | ||
| current_file = self.vim.current.buffer.name |
There was a problem hiding this comment.
I didn't understand the comment here because it still uses self.vim anyway 😩 This change seems to work fine, but let me know if anyone sees an issue.
The cases I believe we know of where using the vim object can be problematic:
- Off of the main thread. The Python Vim API isn't threadsafe so that's why the only code that tries to invoke it from the polling worker thread, which is
disable_plugin, jumps through thethreadsafe_vimhoop (which is just YOLO for standard Vim…). - In
Ensime.__init__. We observed that usingself.vimthere didn't work and my hunch was that's because the class is first being defined when an autocommand likeVimEnterfires triggering it to be loaded, and we're also defining the code that Vim is trying to invoke for our autocommand handler within theEnsimeclass, so there's a reentrancy thing going on there.
Neither of these apply here. It may have been that at one time, further back than I have spelunked in git blame, that current_client was being called from __init__?
There was a problem hiding this comment.
This should be delegated on a method in editor considering we ain't gonna reference vim directly from the client anymore.
There was a problem hiding this comment.
This is a method of the Ensime class, not EnsimeClient—we can discuss this more in the context of design discussion in the second part of these changes, but my reasoning so far is that using vim here is reasonable because the Ensime class basically represents the Vim plugin itself on the Python side, i.e. it's the base class for NeovimEnsime for instance.
There was a problem hiding this comment.
Sorry, my bad for looking at the decontextualized diff. I agree with you on the scope of the vim object.
|
Hey @ches |
|
Hmm that's weird, it works for me. Let me know if you have a chance to figure out more specific reproduction conditions. |
|
Never mind, it seemed to be an unrelated problem which I solved just restarting the server. After commenting here I started noticing problems with other commands, such as |
ensime_shared/editor.py
Outdated
| def set_filetype(self, filetype): | ||
| """Set filetype for the current buffer. | ||
|
|
||
| Unfortunately setting the filetype with ``set_buffer_options`` (or |
There was a problem hiding this comment.
This is an implementation detail, no need to be leaked in the editor API. Maybe it could be specially handled in set_buffer_options instead of being a separate method.
There was a problem hiding this comment.
That's probably true, I guess I thought this might be independently useful, but it could also be automatically handled as a special case in set_buffer_options. There's no other singular form of that anywhere just because it wasn't needed yet.
|
This change is looking so sexy. I've been using it for a week or so and seems to be working well. LGTM You |
|
Thanks for the review and trying it out! I'll incorporate a change of the |
|
Ok. Sounds great. I'll keep using it for a few more days so, just to be sure. But be aware that I only use a small subset of the plugin functionality. |
Syntastic automatically scans the runtimepath for syntax_checkers directories to accommodate plugins: https://github.com/scrooloose/syntastic/wiki/Syntax-Checker-Guide#external The non-standard location was specifically requested in #184, but I don’t agree with the reasoning or the extra code that is required to work around it. I’m not sure Jorge understood that Syntastic has a supported convention. This reverts this commit, but it was squashed away before merge so the object isn’t actually reachable from most clones: ensime/ensime-vim@ed0e941
Get rid of the `commands` dictionary of Vim commands/expressions, giving them a real function API instead of a stringly-typed one. This is also a push toward unit-testability, reducing complex dependencies of objects so they can feasibly be injected with mocks. This is a first pass only, the Editor object API can definitely be improved but most functions are ported intact at this time to make review easier and defer design discussions. Also, a few uses of the `vim` object remain on EnsimeClient -- these are limited to the disable_plugin functionality and will be addressed in #294, it is too complicated to include in the scope of this change. Closes #321.
The original format is supported by lgtm, I like real names personally: https://lgtm.co/docs/maintainers/ This reverts commit d456a13.
|
Okay, I've rebased this and incorporated changes we've discussed. |
We’ve had some recent discussion about what the responsibily of this class should be, and based on that it has some methods that don’t fit or at least should not be public. This is a first pass at cleaning up. Some consideration of the class’s role was hashed out on #328. These changes have eventual relevance to #243, #294, #325.
We’ve had some recent discussion about what the responsibily of this class should be, and based on that it has some methods that don’t fit or at least should not be public. This is a first pass at cleaning up. Some consideration of the class’s role was hashed out on #328. These changes have eventual relevance to #243, #294, #325.
Regression from #328 where Vim's string result to the Syntastic feature detection wasn't properly cast to Boolean, and escaping was doubled after changing match pattern to a Python raw string. Closes #339.
2 participants
This branch is based on my logger and unit testing branches, #323 and #327. It will be a lot easier to review if those get merged. In the meantime here is the diff as if they were already merged:ches/ensime-vim@integration...ches:editor-objectThis is a first pass at factoring all editor functionality out of the
EnsimeClientclass and into a separate one, as described in #321. I would still like to make further improvements to the API of theEditorobject, but since this is already a lot of changes, I've tried to mostly make this phase a "direct port" of existing functions for the benefit of easier review. The follow-up changes might evoke more design discussion, hopefully this two-phase approach helps to defer most of that to when it easier to focus on.The
config.commandsdictionary is completely eliminated 🎉The
vimobject is eliminated fromEnsimeClientwith a couple of exceptions that I'll note inline.New API on the
Editorobject is covered by unit tests mocking thevimobject. I haven't written tests for the old API yet because I want to change a lot of it in the next stage. By the way,make coverageis a thing now.I would appreciate it if brave souls can try running this branch for their work for a few days (cc @ChrisCoffey @ktonga). This is a lot of changes on top of two already significant branches, so I would really like to start landing some of it—I think altogether they are big steps in making ensime-vim easier for contributors to dive into.