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 Jan 6, 2017

We were jumping through some weird hoops to avoid early imports that might have prevented the plugin from loading entirely, with bad error messaging. But that resulted in the plugin getting a lot further into execution than it should (including starting the server), since it can't do anything useful without these deps installed.

I came up with a way to validate the deps before our Python modules load, so now we can import normally in our code. This works in Vim and Neovim, prompting the user with a message when starting up if a dependency is missing.

There is one edge case: if a Neovim user has already used ensime-vim before and has generated the rplugin manifest, Neovim is going to load it. In the case that they've inadvertently removed one of the deps or their Python environment changed or whatever, they'll first be shown the message at startup as usual about missing deps (pausing for them to hit Enter), but then Neovim will start reporting errors about the autocmd handlers not working.

I think this is acceptable, the user still gets the helpful error message first and just needs to quit the editor and fix it. Plus it's a fairly unusual case. What do you think? I can't see a way to avoid this except to have every function in the rplugin do an ImportError check before calling super, or something gross like that. This is an unfortunate consequence of the whole :UpdateRemotePlugins cached manifest system.

@ches ches force-pushed the fail-fast-on-missing-deps branch 3 times, most recently from e9facef to 10896bc Compare January 6, 2017 15:58
We were jumping through some weird hoops to avoid early imports that
might have prevented the plugin from loading entirely, with bad error
messaging. But that resulted in the plugin getting a lot further into
execution than it should (including starting the server), since it can't
do anything useful without these deps installed.

I came up with a way to validate the deps before our Python modules
load, so now we can import normally in our code.

Closes #236
@ches ches force-pushed the fail-fast-on-missing-deps branch from 10896bc to ef161ff Compare January 6, 2017 16:02
self.ensime_server = gconfig["ensime_server"].format(port, uri)
with catch(Exception, disable_completely):
from websocket import create_connection
with catch(websocket.WebSocketException, disable_completely):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bonus benefit: we can catch properly scoped exceptions now in several places, this one should only be a WebSocketException and not also ImportError.

Copy link
Contributor

@ktonga ktonga left a comment

Choose a reason for hiding this comment

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

Nicely done!

I think it's perfectly acceptable, this will help newcomers way more than "restless" users who do weird stuff with their environments :)

On the same page, I have been thinking we could implement a Neovim Health Checker. What you reckon?

@ches
Copy link
Contributor Author

ches commented Jan 6, 2017

On the same page, I have been thinking we could implement a Neovim Health Checker. What you reckon?

That's pretty nifty, I didn't even know the ability for plugins to hook into that got implemented. Too much stuff happening in Neovim for me to keep up with 😄

Okay cool, I'll merge this then because it's going to conflict with #367 so I'll rebase it.

@ches ches merged commit 1b32e85 into ensime:master Jan 6, 2017
@ches ches deleted the fail-fast-on-missing-deps branch January 6, 2017 17:54
@chuwy
Copy link

chuwy commented Jan 7, 2017

For future nvim users, who may experience problems after this update and google for traceback.

After updating all plugins I started to see following warning after git commit:

Error detected while processing function <SNR>98_DependenciesValid:
line   12:
Traceback (most recent call last):
  File "<string>", line 10, in <module>
AttributeError: 'module' object has no attribute 'vars'
Press ENTER or type command to continue
Aborting commit due to empty commit message.

Problem was in ~/gitconfig, because I had vim as core.editor and old vim doesn't know anything about neovim python interface. Solution is just to change core.editor to nvim (and all other possible places).

Sorry if this is irrelevant, but it took some time for me to figure out problem and culprit plugin.

@ches
Copy link
Contributor Author

ches commented Jan 7, 2017

@chuwy That's interesting, this should be compatible with standard Vim's Python interface as well, not Neovim-specific. Can you report the vim --version for the Vim that your git config was executing when this happened?

@chuwy
Copy link

chuwy commented Jan 7, 2017

Hey @ches, sure.

VIM - Vi IMproved 7.3 (2010 Aug 15, compiled Jun 14 2016 16:06:49)
Compiled by root@apple.com
Normal version without GUI.  Features included (+) or not (-):
-arabic +autocmd -balloon_eval -browse +builtin_terms +byte_offset +cindent
-clientserver -clipboard +cmdline_compl +cmdline_hist +cmdline_info +comments
-conceal +cryptv +cscope +cursorbind +cursorshape +dialog_con +diff +digraphs
-dnd -ebcdic -emacs_tags +eval +ex_extra +extra_search -farsi +file_in_path
+find_in_path +float +folding -footer +fork() -gettext -hangul_input +iconv
+insert_expand +jumplist -keymap -langmap +libcall +linebreak +lispindent
+listcmds +localmap -lua +menu +mksession +modify_fname +mouse -mouseshape
-mouse_dec -mouse_gpm -mouse_jsbterm -mouse_netterm -mouse_sysmouse
+mouse_xterm +multi_byte +multi_lang -mzscheme +netbeans_intg -osfiletype
+path_extra -perl +persistent_undo +postscript +printer -profile +python/dyn
-python3 +quickfix +reltime -rightleft +ruby/dyn +scrollbind +signs
+smartindent -sniff +startuptime +statusline -sun_workshop +syntax +tag_binary
+tag_old_static -tag_any_white -tcl +terminfo +termresponse +textobjects +title
 -toolbar +user_commands +vertsplit +virtualedit +visual +visualextra +viminfo
+vreplace +wildignore +wildmenu +windows +writebackup -X11 -xfontset -xim -xsmp
 -xterm_clipboard -xterm_save
   system vimrc file: "$VIM/vimrc"
     user vimrc file: "$HOME/.vimrc"
      user exrc file: "$HOME/.exrc"
  fall-back for $VIM: "/usr/share/vim"
Compilation: gcc -c -I. -D_FORTIFY_SOURCE=0 -Iproto -DHAVE_CONFIG_H -arch i386 -arch x86_64 -g -Os -pipe
Linking: gcc -arch i386 -arch x86_64 -o vim -lncurses

This is standard Mac OS El Capitan vim, placed in /usr/bin/vim.

Obviously this also fails on simple vi invocation, not just on git commit.

@ches
Copy link
Contributor Author

ches commented Jan 7, 2017

Okay thanks, I tried on the stock /usr/bin/vim on Sierra and vim.vars in :py works, maybe this was added in 7.4 because it's VIM - Vi IMproved 7.4 (2013 Aug 10, compiled Sep 19 2016 15:09:36). I'll see about detecting that or using an approach that's more compatible with older versions so others don't run into this error, thanks for reporting.

ches referenced this pull request in ches/ensime-vim Jan 13, 2017
Closes #369, refs #368 (1b32e85)

There are other uses of Vim 7.4+ Python API in our code, this does not
attempt to fix all of them. This just avoids a particularly bad failure
mode during Vim startup.
ches added a commit that referenced this pull request Feb 3, 2017
Closes #369, refs #368 (1b32e85)

There are other uses of Vim 7.4+ Python API in our code, this does not
attempt to fix all of them. This just avoids a particularly bad failure
mode during Vim startup.
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.

3 participants